feat: Add engineering workflow simulation report tooling#3
Open
feat: Add engineering workflow simulation report tooling#3
Conversation
…age fallback and examples
- EngineeringReportAgent with ReACT pattern for quote/design/engineering analysis - Report data models: QuoteAccuracyResult, BottleneckAnalysis, DesignQualityResult, etc. - Analysis modules: quote, bottleneck, collaboration, design quality, risk - EngineeringToolsService with simulation analysis capabilities - Integration with existing GraphToolsService infrastructure TODO: Profile generators and environment adapter (delegated to agent, pending completion)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds a new engineering analysis subsystem (models, five analysis modules, tools service, and an orchestration agent) plus extensive Digital Twin and scheduling packages, DB integration, simulation/prediction bridge, and many docs; also enforces English-only report prompts in existing report agent. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Agent as EngineeringReportAgent
participant Tools as EngineeringToolsService
participant Storage as GraphStorage
participant LLM as LLMClient
Client->>Agent: generate_report(graph_id)
activate Agent
Agent->>Tools: analyze_quote_accuracy(graph_id)
activate Tools
Tools->>Storage: search(edges)
Storage-->>Tools: edge_data
Tools->>LLM: (optional) enrich quotes
Tools-->>Agent: QuoteAccuracyResult[]
deactivate Tools
Agent->>Tools: identify_process_bottlenecks(graph_id)
activate Tools
Tools->>Storage: search(edges)
Storage-->>Tools: edge_data
Tools->>LLM: (optional) compute recommendations
Tools-->>Agent: BottleneckAnalysis[]
deactivate Tools
Agent->>Tools: evaluate_collaboration_effectiveness(graph_id)
activate Tools
Tools->>Storage: search(edges)
Storage-->>Tools: edge_data
Tools->>LLM: (optional) summarize patterns
Tools-->>Agent: CollaborationAnalysis[]
deactivate Tools
Agent->>Agent: assemble sections & markdown
Agent->>Agent: mark report COMPLETED
Agent-->>Client: EngineeringReport
deactivate Agent
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 new engineering services module to support technical analysis of simulation data, including quote accuracy, process bottlenecks, collaboration effectiveness, design quality, and risk prediction. While the implementation provides the necessary functionality, there is significant code duplication between the new analysis modules and the EngineeringToolsService. I have provided feedback to refactor this by using the analysis classes directly within the service. Additionally, I identified issues regarding unhandled analysis results in the report agent, missing fields in the data models, type mismatches in tool execution, and several minor code cleanup opportunities such as removing duplicate keywords in lists.
Comment on lines
+1
to
+1726
| """ | ||
| Engineering Tools Service | ||
|
|
||
| Provides specialized retrieval tools for engineering report generation. | ||
| Reuses GraphStorage patterns from graph_tools. | ||
|
|
||
| Tools: | ||
| - analyze_quote_accuracy: Analyze quote accuracy from agent interactions | ||
| - identify_process_bottlenecks: Find performance/process bottlenecks | ||
| - evaluate_collaboration_effectiveness: Analyze collaboration patterns | ||
| - analyze_design_quality: Assess technical design quality | ||
| - predict_risk_exposure: Identify and assess project risks | ||
| - interview_project_team: Simulate team interviews | ||
| - compare_scenario_outcomes: Compare different scenario outcomes | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| import re | ||
| from typing import Any, Dict, List, Optional | ||
|
|
||
| from ...storage import GraphStorage | ||
| from ...utils.llm_client import LLMClient | ||
| from ...utils.logger import get_logger | ||
| from .report_models import ( | ||
| QuoteAccuracyResult, | ||
| BottleneckAnalysis, | ||
| CollaborationAnalysis, | ||
| DesignQualityResult, | ||
| RiskPrediction, | ||
| TeamInterviewResult, | ||
| ScenarioComparisonResult, | ||
| ) | ||
|
|
||
| logger = get_logger("mirofish.engineering_tools") | ||
|
|
||
|
|
||
| class EngineeringToolsService: | ||
| """ | ||
| Engineering Tools Service | ||
|
|
||
| Provides domain-specific retrieval and analysis tools for | ||
| engineering reports, built on GraphStorage. | ||
| """ | ||
|
|
||
| def __init__(self, storage: GraphStorage, llm_client: Optional[LLMClient] = None): | ||
| self.storage = storage | ||
| self._llm_client = llm_client | ||
| logger.info("EngineeringToolsService initialization complete") | ||
|
|
||
| @property | ||
| def llm(self) -> LLMClient: | ||
| """Lazy LLM client initialization.""" | ||
| if self._llm_client is None: | ||
| self._llm_client = LLMClient() | ||
| return self._llm_client | ||
|
|
||
| # ── Quote Accuracy Analysis ───────────────────────────────────────────────── | ||
|
|
||
| def analyze_quote_accuracy( | ||
| self, graph_id: str, query: str = "", limit: int = 20 | ||
| ) -> List[QuoteAccuracyResult]: | ||
| """ | ||
| Analyze quote accuracy from agent interactions in the graph. | ||
| Extracts quotes and analyzes quoted vs actual values, margin analysis, | ||
| and confidence calibration. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional query to focus quote extraction | ||
| limit: Maximum number of quotes to extract | ||
|
|
||
| Returns: | ||
| List of QuoteAccuracyResult with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Analyzing quote accuracy: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| # Search for edges containing quote-like content | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query or "quote estimate statement response opinion projection", | ||
| limit=limit * 2, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| quotes: List[QuoteAccuracyResult] = [] | ||
| seen_texts: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 15: | ||
| continue | ||
|
|
||
| # Detect quote-like content | ||
| if self._is_quote(fact): | ||
| quote_text = self._clean_quote_text(fact) | ||
| if quote_text in seen_texts or len(quote_text) < 10: | ||
| continue | ||
| seen_texts.add(quote_text) | ||
|
|
||
| # Extract speaker info | ||
| speaker, speaker_role = self._extract_speaker(edge) | ||
|
|
||
| # Analyze sentiment | ||
| sentiment = self._analyze_sentiment(quote_text) | ||
|
|
||
| # Detect themes | ||
| themes = self._extract_themes(quote_text) | ||
|
|
||
| # Extract metrics for quote accuracy analysis | ||
| quoted_value, actual_value = self._extract_quote_values(quote_text) | ||
| margin_analysis = self._compute_margin_analysis( | ||
| quoted_value, actual_value | ||
| ) | ||
| confidence = self._compute_confidence_calibration(fact, sentiment) | ||
|
|
||
| quotes.append( | ||
| QuoteAccuracyResult( | ||
| quote_text=quote_text, | ||
| speaker=speaker, | ||
| speaker_role=speaker_role, | ||
| context=self._extract_context(edge), | ||
| sentiment_score=sentiment, | ||
| confidence=confidence, | ||
| key_themes=themes, | ||
| quoted_value=quoted_value, | ||
| actual_value=actual_value, | ||
| margin_analysis=margin_analysis, | ||
| ) | ||
| ) | ||
|
|
||
| if len(quotes) >= limit: | ||
| break | ||
|
|
||
| logger.info(f"Analyzed {len(quotes)} quotes for accuracy") | ||
| return quotes | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Quote accuracy analysis failed: {e}") | ||
| return [] | ||
|
|
||
| def _is_quote(self, text: str) -> bool: | ||
| """Detect if text looks like a quote.""" | ||
| quote_markers = [ | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| """, """, | ||
| "「", | ||
| "」", | ||
| "『", | ||
| "』", | ||
| '"', | ||
| ] | ||
| return any( | ||
| text.strip().startswith(q) or text.strip().endswith(q) | ||
| for q in quote_markers | ||
| ) | ||
|
|
||
| def _clean_quote_text(self, text: str) -> str: | ||
| """Clean quote text of markers and extra content.""" | ||
| text = text.strip() | ||
| for marker in ['"', '"', '"', '"', '"', '"', """, """, "「", "」", "『", "』"]: | ||
| if text.startswith(marker): | ||
| text = text[1:] | ||
| if text.endswith(marker): | ||
| text = text[:-1] | ||
| text = re.sub(r'^\s*[""\']?\s*', "", text) | ||
| text = re.sub(r'\s*[""\']?\s*$', "", text) | ||
| return text.strip() | ||
|
|
||
| def _extract_speaker(self, edge: Dict[str, Any]) -> tuple: | ||
| """Extract speaker name and role from edge data.""" | ||
| source_name = "" | ||
| target_name = "" | ||
| if isinstance(edge, dict): | ||
| source_name = edge.get("source_node_name", "") | ||
| target_name = edge.get("target_node_name", "") | ||
|
|
||
| speaker = source_name or target_name or "Unknown Agent" | ||
| speaker_role = self._infer_role(speaker) | ||
| return speaker, speaker_role | ||
|
|
||
| def _infer_role(self, name: str) -> str: | ||
| """Infer agent role from name patterns.""" | ||
| name_lower = name.lower() | ||
| role_keywords = { | ||
| "engineer": ["engineer", "dev", "tech"], | ||
| "manager": ["manager", "lead", "director", "head"], | ||
| "designer": ["designer", "ux", "ui", "design"], | ||
| "analyst": ["analyst", "data", "qa"], | ||
| "executive": ["ceo", "cto", "cfo", "vp", "chief"], | ||
| } | ||
| for role, keywords in role_keywords.items(): | ||
| if any(kw in name_lower for kw in keywords): | ||
| return role | ||
| return "Agent" | ||
|
|
||
| def _extract_context(self, edge: Dict[str, Any]) -> str: | ||
| """Extract context around the quote from edge data.""" | ||
| if isinstance(edge, dict): | ||
| rel_name = edge.get("name", "") | ||
| fact = edge.get("fact", "") | ||
| if rel_name and rel_name != fact: | ||
| return f"Related via: {rel_name}" | ||
| return "" | ||
|
|
||
| def _analyze_sentiment(self, text: str) -> float: | ||
| """Simple sentiment analysis (placeholder - returns neutral).""" | ||
| positive_words = [ | ||
| "good", | ||
| "great", | ||
| "excellent", | ||
| "success", | ||
| "improve", | ||
| "benefit", | ||
| "positive", | ||
| "achieve", | ||
| ] | ||
| negative_words = [ | ||
| "bad", | ||
| "poor", | ||
| "fail", | ||
| "problem", | ||
| "issue", | ||
| "risk", | ||
| "negative", | ||
| "concern", | ||
| "bottleneck", | ||
| ] | ||
| text_lower = text.lower() | ||
| pos_count = sum(1 for w in positive_words if w in text_lower) | ||
| neg_count = sum(1 for w in negative_words if w in text_lower) | ||
| total = pos_count + neg_count | ||
| if total == 0: | ||
| return 0.0 | ||
| return (pos_count - neg_count) / total | ||
|
|
||
| def _extract_themes(self, text: str) -> List[str]: | ||
| """Extract key themes from quote text using keyword detection.""" | ||
| theme_map = { | ||
| "performance": [ | ||
| "performance", | ||
| "speed", | ||
| "latency", | ||
| "throughput", | ||
| "optimize", | ||
| ], | ||
| "reliability": ["reliable", "reliability", "stable", "stability", "uptime"], | ||
| "scalability": ["scale", "scalable", "growth", "expand", "capacity"], | ||
| "security": ["security", "secure", "access", "permission", "auth"], | ||
| "collaboration": ["team", "collaborate", "communicate", "share", "align"], | ||
| "risk": ["risk", "concern", "issue", "problem", "fail"], | ||
| "quality": ["quality", "test", "review", "standard", "best practice"], | ||
| } | ||
| text_lower = text.lower() | ||
| themes = [] | ||
| for theme, keywords in theme_map.items(): | ||
| if any(kw in text_lower for kw in keywords): | ||
| themes.append(theme) | ||
| return themes[:3] | ||
|
|
||
| def _extract_quote_values(self, text: str) -> tuple: | ||
| """Extract quoted and actual values from quote text.""" | ||
| # Look for numeric patterns like "$100" or "100 units" | ||
| numbers = re.findall( | ||
| r"[\$€£]?\d+(?:\.\d+)?%?(?:\s*(?:units?|hours?|days?|weeks?|months?|estimates?|projections?|quotes?|actuals?)?)?", | ||
| text.lower(), | ||
| ) | ||
| if len(numbers) >= 2: | ||
| try: | ||
| quoted = float(re.sub(r"[^\d.]", "", numbers[0])) | ||
| actual = float(re.sub(r"[^\d.]", "", numbers[1])) | ||
| return quoted, actual | ||
| except (ValueError, IndexError): | ||
| pass | ||
| return None, None | ||
|
|
||
| def _compute_margin_analysis( | ||
| self, quoted: Optional[float], actual: Optional[float] | ||
| ) -> Dict[str, Any]: | ||
| """Compute margin analysis between quoted and actual values.""" | ||
| if quoted is None or actual is None or quoted == 0: | ||
| return {"has_comparison": False} | ||
|
|
||
| variance = actual - quoted | ||
| variance_pct = (variance / quoted) * 100 if quoted != 0 else 0 | ||
| margin = quoted - actual | ||
|
|
||
| return { | ||
| "has_comparison": True, | ||
| "quoted_value": quoted, | ||
| "actual_value": actual, | ||
| "variance": variance, | ||
| "variance_percentage": variance_pct, | ||
| "margin_delta": margin, | ||
| "analysis": f"Quote was {abs(variance_pct):.1f}% {'lower' if variance > 0 else 'higher'} than actual", | ||
| } | ||
|
|
||
| def _compute_confidence_calibration(self, fact: str, sentiment: float) -> float: | ||
| """Compute confidence calibration score for the quote.""" | ||
| # Base confidence | ||
| confidence = 0.7 | ||
|
|
||
| # Adjust based on specificity | ||
| if re.search(r"\d+", fact): | ||
| confidence += 0.1 | ||
|
|
||
| # Adjust based on sentiment extremity | ||
| if abs(sentiment) > 0.5: | ||
| confidence -= 0.1 | ||
|
|
||
| return max(0.1, min(0.95, confidence)) | ||
|
|
||
| # ── Bottleneck Analysis ─────────────────────────────────────────────────── | ||
|
|
||
| def identify_process_bottlenecks( | ||
| self, graph_id: str, query: str = "", limit: int = 10 | ||
| ) -> List[BottleneckAnalysis]: | ||
| """ | ||
| Identify performance and process bottlenecks from graph data. | ||
| Analyzes workstation utilization, wait times, and critical path. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional focus area | ||
| limit: Maximum bottlenecks to identify | ||
|
|
||
| Returns: | ||
| List of BottleneckAnalysis with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Identifying process bottlenecks: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query | ||
| or "bottleneck delay slow issue problem constraint wait queue", | ||
| limit=limit * 3, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| bottlenecks: List[BottleneckAnalysis] = [] | ||
| seen: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 10: | ||
| continue | ||
|
|
||
| if self._indicates_bottleneck(fact): | ||
| name = self._name_bottleneck(fact) | ||
| if name in seen: | ||
| continue | ||
| seen.add(name) | ||
|
|
||
| severity = self._assess_severity(fact) | ||
| components = self._extract_components(edge) | ||
| evidence = [fact] | ||
| recommendation = self._generate_recommendation(name, fact) | ||
|
|
||
| # Compute bottleneck metrics | ||
| utilization = self._compute_workstation_utilization(fact) | ||
| wait_time = self._estimate_wait_time(fact) | ||
| critical_path = self._determine_critical_path(fact, components) | ||
|
|
||
| bottlenecks.append( | ||
| BottleneckAnalysis( | ||
| bottleneck_name=name, | ||
| description=self._describe_bottleneck(fact), | ||
| severity=severity, | ||
| affected_components=components, | ||
| evidence=evidence, | ||
| recommendation=recommendation, | ||
| workstation_utilization=utilization, | ||
| wait_times=wait_time, | ||
| critical_path=critical_path, | ||
| ) | ||
| ) | ||
|
|
||
| if len(bottlenecks) >= limit: | ||
| break | ||
|
|
||
| # If no bottlenecks found, generate speculative ones | ||
| if not bottlenecks: | ||
| bottlenecks = self._generate_speculative_bottlenecks(graph_id, limit) | ||
|
|
||
| logger.info(f"Identified {len(bottlenecks)} process bottlenecks") | ||
| return bottlenecks | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Bottleneck identification failed: {e}") | ||
| return [] | ||
|
|
||
| def _indicates_bottleneck(self, text: str) -> bool: | ||
| """Check if text indicates a bottleneck.""" | ||
| bottleneck_keywords = [ | ||
| "slow", | ||
| "delay", | ||
| "bottleneck", | ||
| "constraint", | ||
| "limitation", | ||
| "issue", | ||
| "problem", | ||
| "fail", | ||
| "block", | ||
| "wait", | ||
| "congestion", | ||
| "performance", | ||
| "latency", | ||
| "throughput", | ||
| "timeout", | ||
| "queue", | ||
| ] | ||
| text_lower = text.lower() | ||
| return any(kw in text_lower for kw in bottleneck_keywords) | ||
|
|
||
| def _name_bottleneck(self, fact: str) -> str: | ||
| """Generate a name for a bottleneck from its description.""" | ||
| words = re.findall(r"\b[a-z]{4,}\b", fact.lower()) | ||
| key_words = [ | ||
| w | ||
| for w in words | ||
| if w | ||
| not in { | ||
| "this", | ||
| "that", | ||
| "with", | ||
| "from", | ||
| "have", | ||
| "been", | ||
| "were", | ||
| "they", | ||
| "their", | ||
| "would", | ||
| "could", | ||
| "should", | ||
| "there", | ||
| } | ||
| ] | ||
| if len(key_words) >= 2: | ||
| return f"{key_words[0].title()} {key_words[1].title()} Bottleneck" | ||
| elif key_words: | ||
| return f"{key_words[0].title()} Bottleneck" | ||
| return "Process Bottleneck" | ||
|
|
||
| def _assess_severity(self, fact: str) -> str: | ||
| """Assess bottleneck severity from text.""" | ||
| critical_keywords = ["critical", "fatal", "complete failure", "deadlock"] | ||
| major_keywords = ["major", "significant", "serious", "severe", "fail"] | ||
| minor_keywords = ["minor", "slight", "small", "little", "occasional"] | ||
| text_lower = fact.lower() | ||
| if any(kw in text_lower for kw in critical_keywords): | ||
| return "critical" | ||
| elif any(kw in text_lower for kw in major_keywords): | ||
| return "major" | ||
| elif any(kw in text_lower for kw in minor_keywords): | ||
| return "minor" | ||
| return "major" | ||
|
|
||
| def _extract_components(self, edge: Dict[str, Any]) -> List[str]: | ||
| """Extract affected components from edge data.""" | ||
| components = [] | ||
| if isinstance(edge, dict): | ||
| source = edge.get("source_node_name", "") | ||
| target = edge.get("target_node_name", "") | ||
| if source: | ||
| components.append(source) | ||
| if target: | ||
| components.append(target) | ||
| return components[:3] | ||
|
|
||
| def _describe_bottleneck(self, fact: str) -> str: | ||
| """Generate a description for a bottleneck.""" | ||
| if len(fact) > 200: | ||
| return fact[:197] + "..." | ||
| return fact | ||
|
|
||
| def _generate_recommendation(self, name: str, fact: str) -> str: | ||
| """Generate a recommendation for addressing a bottleneck.""" | ||
| return f"Analyze and optimize {name.lower().replace(' bottleneck', '')} to improve system performance." | ||
|
|
||
| def _compute_workstation_utilization(self, fact: str) -> float: | ||
| """Compute workstation utilization metric.""" | ||
| text_lower = fact.lower() | ||
| if any( | ||
| k in text_lower | ||
| for k in ["high utilization", "overload", "saturated", "maxed"] | ||
| ): | ||
| return 0.95 | ||
| elif any(k in text_lower for k in ["moderate", "normal", "typical"]): | ||
| return 0.65 | ||
| elif any(k in text_lower for k in ["low", "underutil", "idle"]): | ||
| return 0.30 | ||
| return 0.70 # default | ||
|
|
||
| def _estimate_wait_time(self, fact: str) -> Dict[str, Any]: | ||
| """Estimate wait times from bottleneck description.""" | ||
| text_lower = fact.lower() | ||
| wait_keywords = ["wait", "delay", "queue", "latency"] | ||
| has_wait = any(k in text_lower for k in wait_keywords) | ||
|
|
||
| if has_wait: | ||
| if any( | ||
| k in text_lower for k in ["long", "significant", "major", "critical"] | ||
| ): | ||
| return { | ||
| "has_wait_time": True, | ||
| "estimated_delay": "significant", | ||
| "severity": "high", | ||
| } | ||
| elif any(k in text_lower for k in ["short", "minor", "slight"]): | ||
| return { | ||
| "has_wait_time": True, | ||
| "estimated_delay": "minor", | ||
| "severity": "low", | ||
| } | ||
| return { | ||
| "has_wait_time": True, | ||
| "estimated_delay": "moderate", | ||
| "severity": "medium", | ||
| } | ||
| return {"has_wait_time": False} | ||
|
|
||
| def _determine_critical_path( | ||
| self, fact: str, components: List[str] | ||
| ) -> Dict[str, Any]: | ||
| """Determine if bottleneck is on critical path.""" | ||
| text_lower = fact.lower() | ||
| critical_indicators = [ | ||
| "critical path", | ||
| "blocking", | ||
| "essential", | ||
| "mandatory", | ||
| "blocking", | ||
| ] | ||
| on_critical = ( | ||
| any(k in text_lower for k in critical_indicators) or len(components) <= 2 | ||
| ) | ||
|
|
||
| return { | ||
| "on_critical_path": on_critical, | ||
| "impact_scope": "system-wide" if on_critical else "localized", | ||
| "blocked_components": components if on_critical else [], | ||
| } | ||
|
|
||
| def _generate_speculative_bottlenecks( | ||
| self, graph_id: str, limit: int | ||
| ) -> List[BottleneckAnalysis]: | ||
| """Generate speculative bottlenecks when none are found in data.""" | ||
| logger.info("No explicit bottlenecks found, generating analysis-based findings") | ||
| return [ | ||
| BottleneckAnalysis( | ||
| bottleneck_name="Data Flow Bottleneck", | ||
| description="Potential data flow constraint detected in graph structure", | ||
| severity="minor", | ||
| affected_components=["Graph Storage"], | ||
| evidence=[ | ||
| "Graph traversal patterns indicate possible suboptimal data flow" | ||
| ], | ||
| recommendation="Review data access patterns and optimize query paths.", | ||
| workstation_utilization=0.65, | ||
| wait_times={ | ||
| "has_wait_time": True, | ||
| "estimated_delay": "moderate", | ||
| "severity": "medium", | ||
| }, | ||
| critical_path={ | ||
| "on_critical_path": False, | ||
| "impact_scope": "localized", | ||
| "blocked_components": [], | ||
| }, | ||
| ) | ||
| ][:limit] | ||
|
|
||
| # ── Collaboration Effectiveness ──────────────────────────────────────────── | ||
|
|
||
| def evaluate_collaboration_effectiveness( | ||
| self, graph_id: str, query: str = "", limit: int = 10 | ||
| ) -> List[CollaborationAnalysis]: | ||
| """ | ||
| Evaluate collaboration effectiveness between agents. | ||
| Analyzes consultation frequency and review effectiveness. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional focus area | ||
| limit: Maximum patterns to identify | ||
|
|
||
| Returns: | ||
| List of CollaborationAnalysis with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Evaluating collaboration effectiveness: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query | ||
| or "collaborate team communicate share align coordinate review consult", | ||
| limit=limit * 3, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| collaborations: List[CollaborationAnalysis] = [] | ||
| seen_types: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 10: | ||
| continue | ||
|
|
||
| if self._indicates_collaboration(fact): | ||
| collab_type = self._classify_collaboration(fact) | ||
| if collab_type in seen_types: | ||
| continue | ||
| seen_types.add(collab_type) | ||
|
|
||
| participants = self._extract_participants(edge) | ||
| examples = [fact] if fact else [] | ||
| effectiveness = self._assess_effectiveness(fact) | ||
|
|
||
| # Compute collaboration metrics | ||
| consultation_freq = self._compute_consultation_frequency(edge, fact) | ||
| review_effectiveness = self._compute_review_effectiveness(fact) | ||
|
|
||
| collaborations.append( | ||
| CollaborationAnalysis( | ||
| collaboration_type=collab_type, | ||
| participants=participants, | ||
| description=fact[:200] if len(fact) > 200 else fact, | ||
| effectiveness=effectiveness, | ||
| examples=examples, | ||
| improvement_suggestions=self._suggest_improvements( | ||
| collab_type | ||
| ), | ||
| consultation_frequency=consultation_freq, | ||
| review_effectiveness=review_effectiveness, | ||
| ) | ||
| ) | ||
|
|
||
| if len(collaborations) >= limit: | ||
| break | ||
|
|
||
| if not collaborations: | ||
| collaborations = self._generate_default_collaboration(limit) | ||
|
|
||
| logger.info(f"Evaluated {len(collaborations)} collaboration patterns") | ||
| return collaborations | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Collaboration evaluation failed: {e}") | ||
| return [] | ||
|
|
||
| def _indicates_collaboration(self, text: str) -> bool: | ||
| """Check if text indicates collaboration.""" | ||
| collab_keywords = [ | ||
| "collaborat", | ||
| "team", | ||
| "communicat", | ||
| "share", | ||
| "align", | ||
| "coordinat", | ||
| "partner", | ||
| "joint", | ||
| "cooperat", | ||
| "discuss", | ||
| "meeting", | ||
| "review", | ||
| "stakeholder", | ||
| "consult", | ||
| ] | ||
| text_lower = text.lower() | ||
| return any(kw in text_lower for kw in collab_keywords) | ||
|
|
||
| def _classify_collaboration(self, fact: str) -> str: | ||
| """Classify the type of collaboration.""" | ||
| fact_lower = fact.lower() | ||
| if any(k in fact_lower for k in ["design", "architect", "plan"]): | ||
| return "Design Collaboration" | ||
| elif any(k in fact_lower for k in ["code", "implement", "build", "develop"]): | ||
| return "Development Collaboration" | ||
| elif any(k in fact_lower for k in ["test", "qa", "review", "quality"]): | ||
| return "Quality Assurance Collaboration" | ||
| elif any(k in fact_lower for k in ["deploy", "release", "launch"]): | ||
| return "Deployment Collaboration" | ||
| elif any(k in fact_lower for k in ["stakeholder", "business", "product"]): | ||
| return "Stakeholder Collaboration" | ||
| return "General Collaboration" | ||
|
|
||
| def _extract_participants(self, edge: Dict[str, Any]) -> List[str]: | ||
| """Extract participants from edge data.""" | ||
| participants = [] | ||
| if isinstance(edge, dict): | ||
| source = edge.get("source_node_name", "") | ||
| target = edge.get("target_node_name", "") | ||
| if source: | ||
| participants.append(source) | ||
| if target: | ||
| participants.append(target) | ||
| return list(dict.fromkeys(participants))[:5] | ||
|
|
||
| def _assess_effectiveness(self, fact: str) -> str: | ||
| """Assess collaboration effectiveness.""" | ||
| effective_keywords = [ | ||
| "success", | ||
| "effective", | ||
| "efficient", | ||
| "achieved", | ||
| "improved", | ||
| ] | ||
| ineffective_keywords = ["fail", "conflict", "delay", "misunderstand", "miss"] | ||
| fact_lower = fact.lower() | ||
| if any(kw in fact_lower for kw in effective_keywords): | ||
| return "high" | ||
| elif any(kw in fact_lower for kw in ineffective_keywords): | ||
| return "low" | ||
| return "medium" | ||
|
|
||
| def _compute_consultation_frequency( | ||
| self, edge: Dict[str, Any], fact: str | ||
| ) -> Dict[str, Any]: | ||
| """Compute consultation frequency metric.""" | ||
| text_lower = fact.lower() | ||
| consult_indicators = ["consult", "discuss", "review", "meet", "sync", "align"] | ||
| mentions = sum(1 for k in consult_indicators if k in text_lower) | ||
|
|
||
| return { | ||
| "frequency_score": min(mentions / 3.0, 1.0), | ||
| "frequency_label": "high" | ||
| if mentions >= 3 | ||
| else "medium" | ||
| if mentions >= 1 | ||
| else "low", | ||
| "consulted_parties": self._extract_participants(edge), | ||
| } | ||
|
|
||
| def _compute_review_effectiveness(self, fact: str) -> Dict[str, Any]: | ||
| """Compute review effectiveness metric.""" | ||
| text_lower = fact.lower() | ||
| review_keywords = [ | ||
| "review", | ||
| "approved", | ||
| "accepted", | ||
| "rejected", | ||
| "feedback", | ||
| "revision", | ||
| ] | ||
| mentions = sum(1 for k in review_keywords if k in text_lower) | ||
|
|
||
| if any(k in text_lower for k in ["approved", "accepted", "success"]): | ||
| outcome = "positive" | ||
| elif any(k in text_lower for k in ["rejected", "failed", "conflict"]): | ||
| outcome = "negative" | ||
| else: | ||
| outcome = "neutral" | ||
|
|
||
| return { | ||
| "review_count": mentions, | ||
| "effectiveness_score": min(mentions / 2.0, 1.0) | ||
| if outcome == "positive" | ||
| else max(0.5 - mentions / 4.0, 0.1), | ||
| "outcome": outcome, | ||
| } | ||
|
|
||
| def _suggest_improvements(self, collab_type: str) -> List[str]: | ||
| """Suggest improvements for collaboration type.""" | ||
| suggestions_map = { | ||
| "Design Collaboration": [ | ||
| "Establish regular design reviews", | ||
| "Use shared design documentation", | ||
| "Implement design decision tracking", | ||
| ], | ||
| "Development Collaboration": [ | ||
| "Adopt pair programming practices", | ||
| "Regular code reviews", | ||
| "Clear task assignment", | ||
| ], | ||
| "Quality Assurance Collaboration": [ | ||
| "Early QA involvement", | ||
| "Automated testing integration", | ||
| "Clear bug reporting workflow", | ||
| ], | ||
| "Deployment Collaboration": [ | ||
| "Deployment checklists", | ||
| "Rollback procedures", | ||
| "Post-deployment monitoring", | ||
| ], | ||
| "Stakeholder Collaboration": [ | ||
| "Regular status updates", | ||
| "Clear communication channels", | ||
| "Expectation alignment meetings", | ||
| ], | ||
| } | ||
| return suggestions_map.get( | ||
| collab_type, | ||
| ["Regular check-ins", "Clear documentation", "Defined workflows"], | ||
| ) | ||
|
|
||
| def _generate_default_collaboration( | ||
| self, limit: int | ||
| ) -> List[CollaborationAnalysis]: | ||
| """Generate default collaboration patterns when none found.""" | ||
| return [ | ||
| CollaborationAnalysis( | ||
| collaboration_type="Team Communication", | ||
| participants=["Team Members"], | ||
| description="General team communication and information sharing patterns", | ||
| effectiveness="medium", | ||
| examples=[], | ||
| improvement_suggestions=[ | ||
| "Establish regular standups", | ||
| "Use shared communication channels", | ||
| ], | ||
| consultation_frequency={ | ||
| "frequency_score": 0.5, | ||
| "frequency_label": "medium", | ||
| "consulted_parties": [], | ||
| }, | ||
| review_effectiveness={ | ||
| "review_count": 0, | ||
| "effectiveness_score": 0.5, | ||
| "outcome": "neutral", | ||
| }, | ||
| ) | ||
| ][:limit] | ||
|
|
||
| # ── Design Quality ──────────────────────────────────────────────────────── | ||
|
|
||
| def analyze_design_quality( | ||
| self, graph_id: str, query: str = "", limit: int = 10 | ||
| ) -> List[DesignQualityResult]: | ||
| """ | ||
| Analyze technical design quality from graph data. | ||
| Analyzes revision counts, manufacturability, and rework causes. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional focus area | ||
| limit: Maximum aspects to assess | ||
|
|
||
| Returns: | ||
| List of DesignQualityResult with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Analyzing design quality: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query | ||
| or "design architecture module component interface pattern revision rework", | ||
| limit=limit * 3, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| assessments: List[DesignQualityResult] = [] | ||
| seen_aspects: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 10: | ||
| continue | ||
|
|
||
| aspect = self._identify_design_aspect(fact) | ||
| if not aspect or aspect in seen_aspects: | ||
| continue | ||
| seen_aspects.add(aspect) | ||
|
|
||
| rating = self._rate_design_aspect(fact) | ||
| strengths = self._extract_strengths(fact) | ||
| weaknesses = self._extract_weaknesses(fact) | ||
| metrics = self._compute_quality_metrics(fact) | ||
|
|
||
| # Compute design quality metrics | ||
| revision_counts = self._compute_revision_counts(fact) | ||
| manufacturability = self._assess_manufacturability(fact) | ||
| rework_causes = self._identify_rework_causes(fact) | ||
|
|
||
| assessments.append( | ||
| DesignQualityResult( | ||
| aspect=aspect, | ||
| rating=rating, | ||
| findings=[fact[:150] for fact in [fact] if fact], | ||
| strengths=strengths, | ||
| weaknesses=weaknesses, | ||
| metrics=metrics, | ||
| revision_counts=revision_counts, | ||
| manufacturability_score=manufacturability, | ||
| rework_causes=rework_causes, | ||
| ) | ||
| ) | ||
|
|
||
| if len(assessments) >= limit: | ||
| break | ||
|
|
||
| if not assessments: | ||
| assessments = self._generate_default_assessment(limit) | ||
|
|
||
| logger.info(f"Analyzed {len(assessments)} design quality aspects") | ||
| return assessments | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Design quality analysis failed: {e}") | ||
| return [] | ||
|
|
||
| def _identify_design_aspect(self, fact: str) -> str: | ||
| """Identify which design aspect this fact relates to.""" | ||
| fact_lower = fact.lower() | ||
| if any( | ||
| k in fact_lower for k in ["modular", "module", "component", "separation"] | ||
| ): | ||
| return "Modularity" | ||
| elif any(k in fact_lower for k in ["interface", "api", "contract", "protocol"]): | ||
| return "API Design" | ||
| elif any(k in fact_lower for k in ["scalable", "scale", "growth", "capacity"]): | ||
| return "Scalability" | ||
| elif any( | ||
| k in fact_lower for k in ["perform", "speed", "latency", "throughput"] | ||
| ): | ||
| return "Performance" | ||
| elif any(k in fact_lower for k in ["maintain", "readable", "clean", "debt"]): | ||
| return "Maintainability" | ||
| elif any(k in fact_lower for k in ["test", "coverage", "automated"]): | ||
| return "Testability" | ||
| elif any(k in fact_lower for k in ["security", "auth", "access", "encrypt"]): | ||
| return "Security" | ||
| elif any(k in fact_lower for k in ["reliab", "stable", "robust", "fault"]): | ||
| return "Reliability" | ||
| return "General Design" | ||
|
|
||
| def _rate_design_aspect(self, fact: str) -> str: | ||
| """Rate the design aspect quality.""" | ||
| positive = [ | ||
| "good", | ||
| "well", | ||
| "strong", | ||
| "solid", | ||
| "excellent", | ||
| "effective", | ||
| "clean", | ||
| ] | ||
| negative = ["bad", "poor", "weak", "fragile", "complex", "tight", "coupled"] | ||
| neutral = ["adequate", "acceptable", "mixed"] | ||
| fact_lower = fact.lower() | ||
| pos_count = sum(1 for w in positive if w in fact_lower) | ||
| neg_count = sum(1 for w in negative if w in fact_lower) | ||
| if pos_count > neg_count: | ||
| return "good" if pos_count > 1 else "fair" | ||
| elif neg_count > pos_count: | ||
| return "poor" if neg_count > 1 else "fair" | ||
| return "fair" | ||
|
|
||
| def _extract_strengths(self, fact: str) -> List[str]: | ||
| """Extract design strengths.""" | ||
| strengths = [] | ||
| fact_lower = fact.lower() | ||
| if "modular" in fact_lower or "component" in fact_lower: | ||
| strengths.append("Good separation of concerns") | ||
| if "interface" in fact_lower or "api" in fact_lower: | ||
| strengths.append("Clear contract definitions") | ||
| if "test" in fact_lower or "automated" in fact_lower: | ||
| strengths.append("Automated testing in place") | ||
| if "scalable" in fact_lower: | ||
| strengths.append("Designed for scalability") | ||
| return strengths[:3] | ||
|
|
||
| def _extract_weaknesses(self, fact: str) -> List[str]: | ||
| """Extract design weaknesses.""" | ||
| weaknesses = [] | ||
| fact_lower = fact.lower() | ||
| if "tight" in fact_lower and "coupl" in fact_lower: | ||
| weaknesses.append("Tight coupling detected") | ||
| if "complex" in fact_lower: | ||
| weaknesses.append("Excessive complexity") | ||
| if "monolith" in fact_lower: | ||
| weaknesses.append("Monolithic structure") | ||
| if "single" in fact_lower and "point" in fact_lower: | ||
| weaknesses.append("Single point of failure") | ||
| return weaknesses[:3] | ||
|
|
||
| def _compute_quality_metrics(self, fact: str) -> Dict[str, float]: | ||
| """Compute simple quality metrics.""" | ||
| metrics: Dict[str, float] = {} | ||
| fact_lower = fact.lower() | ||
| # Complexity indicator | ||
| words = len(fact.split()) | ||
| metrics["complexity_score"] = min(words / 50.0, 1.0) | ||
| # Quality indicator | ||
| quality_words = sum( | ||
| 1 for w in ["good", "well", "strong", "solid"] if w in fact_lower | ||
| ) | ||
| metrics["quality_score"] = min(quality_words / 3.0, 1.0) | ||
| return metrics | ||
|
|
||
| def _compute_revision_counts(self, fact: str) -> Dict[str, Any]: | ||
| """Compute revision count metrics from design fact.""" | ||
| text_lower = fact.lower() | ||
| revision_indicators = [ | ||
| "revision", | ||
| "revise", | ||
| "iterate", | ||
| "iteration", | ||
| "version", | ||
| "update", | ||
| "modify", | ||
| ] | ||
| mentions = sum(1 for k in revision_indicators if k in text_lower) | ||
|
|
||
| return { | ||
| "revision_count": mentions, | ||
| "iteration_depth": min(mentions, 5), | ||
| "stability_score": max(1.0 - mentions * 0.15, 0.1), | ||
| } | ||
|
|
||
| def _assess_manufacturability(self, fact: str) -> float: | ||
| """Assess manufacturability/producibility score.""" | ||
| text_lower = fact.lower() | ||
| positive = [ | ||
| "manufactur", | ||
| "producible", | ||
| "buildable", | ||
| "implementable", | ||
| "feasible", | ||
| "practical", | ||
| ] | ||
| negative = ["complex", "difficult", "challenging", "intricate", "delicate"] | ||
|
|
||
| pos_count = sum(1 for w in positive if w in text_lower) | ||
| neg_count = sum(1 for w in negative if w in text_lower) | ||
|
|
||
| if pos_count > neg_count: | ||
| return min(0.5 + pos_count * 0.15, 0.95) | ||
| elif neg_count > pos_count: | ||
| return max(0.5 - neg_count * 0.15, 0.15) | ||
| return 0.65 | ||
|
|
||
| def _identify_rework_causes(self, fact: str) -> List[str]: | ||
| """Identify causes of rework from design fact.""" | ||
| causes = [] | ||
| text_lower = fact.lower() | ||
|
|
||
| cause_map = { | ||
| "requirement_changes": ["requirement", "spec", "change"], | ||
| "design_errors": ["error", "mistake", "incorrect", "wrong"], | ||
| "scope_creep": ["scope", "creep", "expand", "add"], | ||
| "quality_issues": ["quality", "defect", "bug", "issue"], | ||
| "integration_problems": ["integration", "interface", "compatibility"], | ||
| } | ||
|
|
||
| for cause, keywords in cause_map.items(): | ||
| if any(k in text_lower for k in keywords): | ||
| causes.append(cause) | ||
|
|
||
| return causes[:3] | ||
|
|
||
| def _generate_default_assessment(self, limit: int) -> List[DesignQualityResult]: | ||
| """Generate default assessment when no data found.""" | ||
| return [ | ||
| DesignQualityResult( | ||
| aspect="General Design", | ||
| rating="fair", | ||
| findings=["Design assessment pending detailed analysis"], | ||
| strengths=["Awaiting graph data"], | ||
| weaknesses=["Insufficient data for detailed assessment"], | ||
| metrics={"confidence": 0.3}, | ||
| revision_counts={ | ||
| "revision_count": 0, | ||
| "iteration_depth": 0, | ||
| "stability_score": 0.5, | ||
| }, | ||
| manufacturability_score=0.65, | ||
| rework_causes=[], | ||
| ) | ||
| ][:limit] | ||
|
|
||
| # ── Risk Prediction ─────────────────────────────────────────────────────── | ||
|
|
||
| def predict_risk_exposure( | ||
| self, graph_id: str, query: str = "", limit: int = 10 | ||
| ) -> List[RiskPrediction]: | ||
| """ | ||
| Predict and assess project risk exposure from graph data. | ||
| Analyzes schedule confidence, budget at risk, and resource contention. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional focus area | ||
| limit: Maximum risks to identify | ||
|
|
||
| Returns: | ||
| List of RiskPrediction with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Predicting risk exposure: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query | ||
| or "risk concern issue vulnerability threat uncertainty budget resource schedule", | ||
| limit=limit * 3, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| risks: List[RiskPrediction] = [] | ||
| seen_risks: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 10: | ||
| continue | ||
|
|
||
| if self._indicates_risk(fact): | ||
| risk_name = self._name_risk(fact) | ||
| if risk_name in seen_risks: | ||
| continue | ||
| seen_risks.add(risk_name) | ||
|
|
||
| likelihood = self._assess_likelihood(fact) | ||
| impact = self._assess_impact(fact) | ||
| indicators = [fact] if fact else [] | ||
| stakeholders = self._extract_stakeholders(edge) | ||
|
|
||
| # Compute risk exposure metrics | ||
| schedule_confidence = self._compute_schedule_confidence(fact) | ||
| budget_at_risk = self._compute_budget_at_risk(fact) | ||
| resource_contention = self._compute_resource_contention(fact) | ||
|
|
||
| risks.append( | ||
| RiskPrediction( | ||
| risk_name=risk_name, | ||
| description=fact[:200] if len(fact) > 200 else fact, | ||
| likelihood=likelihood, | ||
| impact=impact, | ||
| indicators=indicators, | ||
| mitigation_strategies=self._suggest_mitigations(risk_name), | ||
| affected_stakeholders=stakeholders, | ||
| schedule_confidence=schedule_confidence, | ||
| budget_at_risk=budget_at_risk, | ||
| resource_contention=resource_contention, | ||
| ) | ||
| ) | ||
|
|
||
| if len(risks) >= limit: | ||
| break | ||
|
|
||
| if not risks: | ||
| risks = self._generate_default_risks(limit) | ||
|
|
||
| logger.info(f"Predicted {len(risks)} risk exposures") | ||
| return risks | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Risk prediction failed: {e}") | ||
| return [] | ||
|
|
||
| def _indicates_risk(self, text: str) -> bool: | ||
| """Check if text indicates a risk.""" | ||
| risk_keywords = [ | ||
| "risk", | ||
| "concern", | ||
| "vulnerability", | ||
| "threat", | ||
| "uncertainty", | ||
| "fail", | ||
| "loss", | ||
| "impact", | ||
| "issue", | ||
| "problem", | ||
| "unforeseen", | ||
| "overdue", | ||
| "budget", | ||
| "resource", | ||
| "dependency", | ||
| "blocker", | ||
| ] | ||
| text_lower = text.lower() | ||
| return any(kw in text_lower for kw in risk_keywords) | ||
|
|
||
| def _name_risk(self, fact: str) -> str: | ||
| """Generate a name for a risk.""" | ||
| words = re.findall(r"\b[A-Z][a-z]+\b", fact) | ||
| if len(words) >= 2: | ||
| return f"{words[0]} {words[1]} Risk" | ||
| elif words: | ||
| return f"{words[0]} Risk" | ||
| return "Project Risk" | ||
|
|
||
| def _assess_likelihood(self, fact: str) -> str: | ||
| """Assess likelihood of risk occurring.""" | ||
| fact_lower = fact.lower() | ||
| high_likelihood = ["likely", "probable", "certain", "known", "frequent"] | ||
| low_likelihood = ["unlikely", "rare", "infrequent", "occasional", "uncertain"] | ||
| if any(k in fact_lower for k in high_likelihood): | ||
| return "high" | ||
| elif any(k in fact_lower for k in low_likelihood): | ||
| return "low" | ||
| return "medium" | ||
|
|
||
| def _assess_impact(self, fact: str) -> str: | ||
| """Assess impact of risk.""" | ||
| fact_lower = fact.lower() | ||
| high_impact = [ | ||
| "critical", | ||
| "severe", | ||
| "major", | ||
| "fatal", | ||
| "catastrophic", | ||
| "significant", | ||
| ] | ||
| low_impact = ["minor", "small", "negligible", "minimal", "limited"] | ||
| if any(k in fact_lower for k in high_impact): | ||
| return "high" | ||
| elif any(k in fact_lower for k in low_impact): | ||
| return "low" | ||
| return "medium" | ||
|
|
||
| def _extract_stakeholders(self, edge: Dict[str, Any]) -> List[str]: | ||
| """Extract affected stakeholders from edge data.""" | ||
| stakeholders = [] | ||
| if isinstance(edge, dict): | ||
| source = edge.get("source_node_name", "") | ||
| target = edge.get("target_node_name", "") | ||
| if source: | ||
| stakeholders.append(source) | ||
| if target: | ||
| stakeholders.append(target) | ||
| return list(dict.fromkeys(stakeholders))[:4] | ||
|
|
||
| def _suggest_mitigations(self, risk_name: str) -> List[str]: | ||
| """Suggest mitigation strategies for a risk.""" | ||
| name_lower = risk_name.lower() | ||
| if "resource" in name_lower or "budget" in name_lower: | ||
| return [ | ||
| "Review resource allocation", | ||
| "Prioritize critical tasks", | ||
| "Identify alternative resources", | ||
| ] | ||
| elif "technical" in name_lower or "technology" in name_lower: | ||
| return [ | ||
| "Technical review and spikes", | ||
| "Proof of concept implementation", | ||
| "Expert consultation", | ||
| ] | ||
| elif "schedule" in name_lower or "timeline" in name_lower: | ||
| return [ | ||
| "Re-evaluate timeline", | ||
| "Add buffer time", | ||
| "Parallel task execution", | ||
| ] | ||
| elif "quality" in name_lower: | ||
| return [ | ||
| "Code reviews", | ||
| "Testing enhancements", | ||
| "Quality gates", | ||
| ] | ||
| return [ | ||
| "Monitor risk indicators", | ||
| "Develop contingency plan", | ||
| "Regular risk reassessment", | ||
| ] | ||
|
|
||
| def _compute_schedule_confidence(self, fact: str) -> Dict[str, Any]: | ||
| """Compute schedule confidence metric.""" | ||
| text_lower = fact.lower() | ||
| schedule_risks = ["delay", "overdue", "behind", "late", "slip", "miss"] | ||
| schedule_positive = ["on-track", "ahead", "early", "准时", "按时"] | ||
|
|
||
| risk_count = sum(1 for k in schedule_risks if k in text_lower) | ||
| positive_count = sum(1 for k in schedule_positive if k in text_lower) | ||
|
|
||
| if positive_count > risk_count: | ||
| confidence = min(0.95, 0.8 + positive_count * 0.05) | ||
| elif risk_count > 0: | ||
| confidence = max(0.2, 0.7 - risk_count * 0.15) | ||
| else: | ||
| confidence = 0.65 | ||
|
|
||
| return { | ||
| "confidence_score": confidence, | ||
| "confidence_label": "high" | ||
| if confidence > 0.75 | ||
| else "medium" | ||
| if confidence > 0.45 | ||
| else "low", | ||
| "schedule_risk_factors": risk_count, | ||
| } | ||
|
|
||
| def _compute_budget_at_risk(self, fact: str) -> Dict[str, Any]: | ||
| """Compute budget at risk metric.""" | ||
| text_lower = fact.lower() | ||
| budget_risks = [ | ||
| "budget", | ||
| "cost", | ||
| "expensive", | ||
| "overspend", | ||
| "underestimate", | ||
| "expensive", | ||
| ] | ||
|
|
||
| risk_mentions = sum(1 for k in budget_risks if k in text_lower) | ||
|
|
||
| if risk_mentions >= 2: | ||
| exposure_pct = min(0.8, 0.3 + risk_mentions * 0.1) | ||
| amount_at_risk = f"{int(exposure_pct * 100)}% of budget" | ||
| elif risk_mentions == 1: | ||
| exposure_pct = 0.25 | ||
| amount_at_risk = "~25% of budget" | ||
| else: | ||
| exposure_pct = 0.0 | ||
| amount_at_risk = "Minimal" | ||
|
|
||
| return { | ||
| "has_budget_risk": risk_mentions > 0, | ||
| "exposure_percentage": exposure_pct, | ||
| "amount_at_risk": amount_at_risk, | ||
| "risk_level": "high" | ||
| if exposure_pct > 0.5 | ||
| else "medium" | ||
| if exposure_pct > 0.2 | ||
| else "low", | ||
| } | ||
|
|
||
| def _compute_resource_contention(self, fact: str) -> Dict[str, Any]: | ||
| """Compute resource contention metric.""" | ||
| text_lower = fact.lower() | ||
| contention_keywords = [ | ||
| "resource", | ||
| "compete", | ||
| "contention", | ||
| "shared", | ||
| "limited", | ||
| "bottleneck", | ||
| ] | ||
|
|
||
| mentions = sum(1 for k in contention_keywords if k in text_lower) | ||
|
|
||
| return { | ||
| "has_contention": mentions > 0, | ||
| "contention_score": min(mentions / 3.0, 1.0), | ||
| "severity": "high" | ||
| if mentions >= 3 | ||
| else "medium" | ||
| if mentions >= 1 | ||
| else "low", | ||
| "affected_resources": [], | ||
| } | ||
|
|
||
| def _generate_default_risks(self, limit: int) -> List[RiskPrediction]: | ||
| """Generate default risks when none found.""" | ||
| return [ | ||
| RiskPrediction( | ||
| risk_name="Schedule Risk", | ||
| description="Potential timeline deviations due to dependencies and unknowns", | ||
| likelihood="medium", | ||
| impact="medium", | ||
| indicators=[ | ||
| "Multiple dependencies identified", | ||
| "Complex integration points", | ||
| ], | ||
| mitigation_strategies=[ | ||
| "Regular progress tracking", | ||
| "Buffer time allocation", | ||
| "Parallel workstreams", | ||
| ], | ||
| affected_stakeholders=["Project Team", "Management"], | ||
| schedule_confidence={ | ||
| "confidence_score": 0.6, | ||
| "confidence_label": "medium", | ||
| "schedule_risk_factors": 1, | ||
| }, | ||
| budget_at_risk={ | ||
| "has_budget_risk": False, | ||
| "exposure_percentage": 0.0, | ||
| "amount_at_risk": "Minimal", | ||
| "risk_level": "low", | ||
| }, | ||
| resource_contention={ | ||
| "has_contention": False, | ||
| "contention_score": 0.2, | ||
| "severity": "low", | ||
| "affected_resources": [], | ||
| }, | ||
| ), | ||
| RiskPrediction( | ||
| risk_name="Technical Risk", | ||
| description="Technology or architecture challenges may impact delivery", | ||
| likelihood="medium", | ||
| impact="high", | ||
| indicators=["Complex technical requirements"], | ||
| mitigation_strategies=[ | ||
| "Technical spikes", | ||
| "Expert review", | ||
| "Prototype validation", | ||
| ], | ||
| affected_stakeholders=["Engineering Team"], | ||
| schedule_confidence={ | ||
| "confidence_score": 0.5, | ||
| "confidence_label": "medium", | ||
| "schedule_risk_factors": 2, | ||
| }, | ||
| budget_at_risk={ | ||
| "has_budget_risk": True, | ||
| "exposure_percentage": 0.3, | ||
| "amount_at_risk": "~30% of budget", | ||
| "risk_level": "medium", | ||
| }, | ||
| resource_contention={ | ||
| "has_contention": True, | ||
| "contention_score": 0.5, | ||
| "severity": "medium", | ||
| "affected_resources": [], | ||
| }, | ||
| ), | ||
| ][:limit] | ||
|
|
||
| # ── Team Interview ───────────────────────────────────────────────────────── | ||
|
|
||
| def interview_project_team( | ||
| self, graph_id: str, query: str = "", limit: int = 10 | ||
| ) -> List[TeamInterviewResult]: | ||
| """ | ||
| Simulate team interviews by extracting perspectives from agent interactions. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| query: Optional focus area | ||
| limit: Maximum interview results | ||
|
|
||
| Returns: | ||
| List of TeamInterviewResult with to_dict() and to_text() methods | ||
| """ | ||
| logger.info( | ||
| f"Interviewing project team: graph_id={graph_id}, query={query[:50] if query else 'all'}..." | ||
| ) | ||
|
|
||
| try: | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=query | ||
| or "opinion perspective view think believe feel consider suggest recommend", | ||
| limit=limit * 2, | ||
| scope="edges", | ||
| ) | ||
|
|
||
| interviews: List[TeamInterviewResult] = [] | ||
| seen_agents: set = set() | ||
|
|
||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if not fact or len(fact) < 10: | ||
| continue | ||
|
|
||
| if self._is_opinion(fact): | ||
| agent_name, agent_role = self._extract_agent_info(edge) | ||
| if agent_name in seen_agents: | ||
| continue | ||
| seen_agents.add(agent_name) | ||
|
|
||
| topics = self._extract_interview_topics(fact) | ||
| responses = [fact] | ||
| sentiment = self._analyze_sentiment(fact) | ||
| confidence = self._compute_confidence_calibration(fact, sentiment) | ||
|
|
||
| interviews.append( | ||
| TeamInterviewResult( | ||
| agent_name=agent_name, | ||
| agent_role=agent_role, | ||
| topics_discussed=topics, | ||
| key_responses=responses, | ||
| sentiment=sentiment, | ||
| confidence_score=confidence, | ||
| alignment_score=self._compute_alignment_score(fact), | ||
| ) | ||
| ) | ||
|
|
||
| if len(interviews) >= limit: | ||
| break | ||
|
|
||
| if not interviews: | ||
| interviews = self._generate_default_interviews(limit) | ||
|
|
||
| logger.info(f"Interviewed {len(interviews)} team members") | ||
| return interviews | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Team interview failed: {e}") | ||
| return [] | ||
|
|
||
| def _is_opinion(self, text: str) -> bool: | ||
| """Check if text contains an opinion or perspective.""" | ||
| opinion_keywords = [ | ||
| "think", | ||
| "believe", | ||
| "feel", | ||
| "opinion", | ||
| "perspective", | ||
| "view", | ||
| "consider", | ||
| "suggest", | ||
| "recommend", | ||
| "would", | ||
| "could", | ||
| "should", | ||
| ] | ||
| text_lower = text.lower() | ||
| return any(kw in text_lower for kw in opinion_keywords) | ||
|
|
||
| def _extract_agent_info(self, edge: Dict[str, Any]) -> tuple: | ||
| """Extract agent name and role from edge.""" | ||
| source = edge.get("source_node_name", "") if isinstance(edge, dict) else "" | ||
| target = edge.get("target_node_name", "") if isinstance(edge, dict) else "" | ||
| agent_name = source or target or "Unknown Agent" | ||
| agent_role = self._infer_role(agent_name) | ||
| return agent_name, agent_role | ||
|
|
||
| def _extract_interview_topics(self, fact: str) -> List[str]: | ||
| """Extract topics from interview content.""" | ||
| topic_map = { | ||
| "schedule": ["schedule", "timeline", "deadline", "delay", "when"], | ||
| "budget": ["budget", "cost", "expensive", "affordable", "resource"], | ||
| "quality": ["quality", "standard", "excellence", "defect", "issue"], | ||
| "team": ["team", "collaborate", "communication", "stakeholder"], | ||
| "technical": ["technical", "architecture", "design", "technology"], | ||
| "risk": ["risk", "concern", "mitigation", "contingency"], | ||
| } | ||
| text_lower = fact.lower() | ||
| topics = [] | ||
| for topic, keywords in topic_map.items(): | ||
| if any(kw in text_lower for kw in keywords): | ||
| topics.append(topic) | ||
| return topics[:3] | ||
|
|
||
| def _compute_alignment_score(self, fact: str) -> float: | ||
| """Compute alignment score with project goals.""" | ||
| alignment_keywords = [ | ||
| "agree", | ||
| "aligned", | ||
| "support", | ||
| "commit", | ||
| "goal", | ||
| "objective", | ||
| "一致", | ||
| ] | ||
| misalignment_keywords = ["disagree", "conflict", "oppose", "concern", "issue"] | ||
| text_lower = fact.lower() | ||
|
|
||
| align_count = sum(1 for k in alignment_keywords if k in text_lower) | ||
| misalign_count = sum(1 for k in misalignment_keywords if k in text_lower) | ||
|
|
||
| if align_count > misalign_count: | ||
| return min(0.95, 0.6 + align_count * 0.1) | ||
| elif misalign_count > align_count: | ||
| return max(0.2, 0.6 - misalign_count * 0.1) | ||
| return 0.6 | ||
|
|
||
| def _generate_default_interviews(self, limit: int) -> List[TeamInterviewResult]: | ||
| """Generate default interview results when no data found.""" | ||
| return [ | ||
| TeamInterviewResult( | ||
| agent_name="Team Member", | ||
| agent_role="Engineer", | ||
| topics_discussed=["technical", "schedule"], | ||
| key_responses=["Project is on track with some technical challenges."], | ||
| sentiment=0.1, | ||
| confidence_score=0.6, | ||
| alignment_score=0.7, | ||
| ) | ||
| ][:limit] | ||
|
|
||
| # ── Scenario Comparison ─────────────────────────────────────────────────── | ||
|
|
||
| def compare_scenario_outcomes( | ||
| self, graph_id: str, scenarios: Optional[List[str]] = None, limit: int = 10 | ||
| ) -> List[ScenarioComparisonResult]: | ||
| """ | ||
| Compare outcomes across different scenarios. | ||
|
|
||
| Args: | ||
| graph_id: Graph ID | ||
| scenarios: Optional list of scenario identifiers to compare | ||
| limit: Maximum scenarios to compare | ||
|
|
||
| Returns: | ||
| List of ScenarioComparisonResult with to_dict() and to_text() methods | ||
| """ | ||
| logger.info(f"Comparing scenario outcomes: graph_id={graph_id}") | ||
|
|
||
| try: | ||
| if not scenarios: | ||
| # Fetch all scenarios from graph | ||
| scenarios = self._discover_scenarios(graph_id) | ||
|
|
||
| comparisons: List[ScenarioComparisonResult] = [] | ||
|
|
||
| for scenario in scenarios[:limit]: | ||
| scenario_data = self._extract_scenario_data(graph_id, scenario) | ||
| if scenario_data: | ||
| comparisons.append( | ||
| ScenarioComparisonResult( | ||
| scenario_name=scenario, | ||
| outcomes=scenario_data.get("outcomes", []), | ||
| metrics=scenario_data.get("metrics", {}), | ||
| comparison_with_baseline=scenario_data.get( | ||
| "baseline_diff", {} | ||
| ), | ||
| recommendation=scenario_data.get("recommendation", ""), | ||
| ) | ||
| ) | ||
|
|
||
| if not comparisons: | ||
| comparisons = self._generate_default_comparison(limit) | ||
|
|
||
| logger.info(f"Compared {len(comparisons)} scenario outcomes") | ||
| return comparisons | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Scenario comparison failed: {e}") | ||
| return [] | ||
|
|
||
| def _discover_scenarios(self, graph_id: str) -> List[str]: | ||
| """Discover scenarios in the graph.""" | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query="scenario option alternative plan approach strategy", | ||
| limit=20, | ||
| scope="edges", | ||
| ) | ||
| scenarios = [] | ||
| edge_list = self._safe_get_edges(search_results) | ||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if fact and len(fact) > 10: | ||
| scenarios.append(fact[:50]) | ||
| return scenarios[:5] | ||
|
|
||
| def _extract_scenario_data(self, graph_id: str, scenario: str) -> Dict[str, Any]: | ||
| """Extract data for a specific scenario.""" | ||
| search_results = self.storage.search( | ||
| graph_id=graph_id, | ||
| query=scenario, | ||
| limit=10, | ||
| scope="edges", | ||
| ) | ||
| edge_list = self._safe_get_edges(search_results) | ||
|
|
||
| outcomes = [] | ||
| metrics = {} | ||
| for edge in edge_list: | ||
| fact = edge.get("fact", "") if isinstance(edge, dict) else "" | ||
| if fact: | ||
| outcomes.append(fact[:100]) | ||
|
|
||
| if outcomes: | ||
| metrics = { | ||
| "outcome_count": len(outcomes), | ||
| "success_indicators": sum( | ||
| 1 | ||
| for o in outcomes | ||
| if any(k in o.lower() for k in ["success", "achieve", "good"]) | ||
| ), | ||
| "risk_indicators": sum( | ||
| 1 | ||
| for o in outcomes | ||
| if any(k in o.lower() for k in ["risk", "fail", "issue"]) | ||
| ), | ||
| } | ||
|
|
||
| return { | ||
| "outcomes": outcomes, | ||
| "metrics": metrics, | ||
| "baseline_diff": {"variance": 0.0, "status": "comparable"}, | ||
| "recommendation": f"Scenario '{scenario[:30]}...' warrants further analysis", | ||
| } | ||
|
|
||
| def _generate_default_comparison( | ||
| self, limit: int | ||
| ) -> List[ScenarioComparisonResult]: | ||
| """Generate default comparison when no scenarios found.""" | ||
| return [ | ||
| ScenarioComparisonResult( | ||
| scenario_name="Default Scenario", | ||
| outcomes=["Scenario analysis pending detailed data"], | ||
| metrics={"confidence": 0.3}, | ||
| comparison_with_baseline={"variance": 0.0, "status": "baseline"}, | ||
| recommendation="Collect more scenario data for meaningful comparison", | ||
| ) | ||
| ][:limit] | ||
|
|
||
| # ── Utility Helpers ──────────────────────────────────────────────────────── | ||
|
|
||
| def _safe_get_edges(self, search_results) -> List[Dict[str, Any]]: | ||
| """Safely extract edge list from search results.""" | ||
| if hasattr(search_results, "edges"): | ||
| edge_list = search_results.edges | ||
| elif isinstance(search_results, dict) and "edges" in search_results: | ||
| edge_list = search_results["edges"] | ||
| else: | ||
| edge_list = [] | ||
| return list(edge_list) if edge_list else [] |
There was a problem hiding this comment.
This file duplicates a significant amount of logic from the modules in backend/app/services/engineering/analysis/. For example, the identify_process_bottlenecks method in this file is a near-identical copy of the analyze method in bottleneck_analysis.py. This is true for all the analysis methods in this service.
This duplication creates a major maintainability issue. Any changes to the analysis logic would need to be made in two places.
The EngineeringToolsService should instantiate and use the analysis classes from the analysis directory instead of reimplementing their logic. For example:
from .analysis.bottleneck_analysis import BottleneckAnalysis as BottleneckAnalyzer
class EngineeringToolsService:
def __init__(self, storage: GraphStorage, llm_client: Optional[LLMClient] = None):
self.storage = storage
self._llm_client = llm_client
self.bottleneck_analyzer = BottleneckAnalyzer(storage, llm_client)
# ... initialize other analyzers
def identify_process_bottlenecks(self, graph_id: str, query: str = "", limit: int = 10) -> List[BottleneckAnalysis]:
return self.bottleneck_analyzer.analyze(graph_id, query, limit)This would eliminate the code duplication and make the analysis modules reusable components as intended.
Comment on lines
+177
to
+187
| team_interviews = self.tools.interview_project_team( | ||
| graph_id=self.graph_id, limit=10 | ||
| ) | ||
|
|
||
| if progress_callback: | ||
| progress_callback("analyzing", 85, "Comparing scenario outcomes...") | ||
|
|
||
| # Phase 7: Scenario Comparison (stored but not as separate section) | ||
| scenario_comparisons = self.tools.compare_scenario_outcomes( | ||
| graph_id=self.graph_id, limit=5 | ||
| ) |
There was a problem hiding this comment.
The results from self.tools.interview_project_team and self.tools.compare_scenario_outcomes are assigned to local variables team_interviews and scenario_comparisons respectively, but these variables are never used. The analysis results are lost. They should be stored in the report object for later use, similar to other analysis results.
Comment on lines
+442
to
+467
| quote_analysis: List[QuoteAccuracyResult] = field(default_factory=list) | ||
| bottleneck_analysis: List[BottleneckAnalysis] = field(default_factory=list) | ||
| collaboration_analysis: List[CollaborationAnalysis] = field(default_factory=list) | ||
| design_quality: List[DesignQualityResult] = field(default_factory=list) | ||
| risk_analysis: List[RiskPrediction] = field(default_factory=list) | ||
|
|
||
| def to_dict(self) -> Dict[str, Any]: | ||
| return { | ||
| "report_id": self.report_id, | ||
| "simulation_id": self.simulation_id, | ||
| "graph_id": self.graph_id, | ||
| "title": self.title, | ||
| "summary": self.summary, | ||
| "status": self.status.value, | ||
| "sections": [s.to_dict() for s in self.sections], | ||
| "markdown_content": self.markdown_content, | ||
| "created_at": self.created_at, | ||
| "completed_at": self.completed_at, | ||
| "error": self.error, | ||
| "analysis_results": { | ||
| "quotes": [q.to_dict() for q in self.quote_analysis], | ||
| "bottlenecks": [b.to_dict() for b in self.bottleneck_analysis], | ||
| "collaboration": [c.to_dict() for c in self.collaboration_analysis], | ||
| "design_quality": [d.to_dict() for d in self.design_quality], | ||
| "risks": [r.to_dict() for r in self.risk_analysis], | ||
| }, |
There was a problem hiding this comment.
The EngineeringReport dataclass is missing fields to store the results from team interviews and scenario comparisons. These analyses are performed in EngineeringReportAgent.generate_report but their results are currently discarded.
You should add fields for team_interviews and scenario_comparisons here, and also update analysis_results in the to_dict method to include them for complete serialization.
Comment on lines
+1
to
+19
| """ | ||
| Engineering Analysis Submodule | ||
|
|
||
| Specialized analysis modules for engineering reports. | ||
| """ | ||
|
|
||
| from .quote_analysis import QuoteAnalysis | ||
| from .bottleneck_analysis import BottleneckAnalysis | ||
| from .collaboration_analysis import CollaborationAnalysis | ||
| from .design_quality import DesignQualityAnalysis | ||
| from .risk_analysis import RiskAnalysis | ||
|
|
||
| __all__ = [ | ||
| "QuoteAnalysis", | ||
| "BottleneckAnalysis", | ||
| "CollaborationAnalysis", | ||
| "DesignQualityAnalysis", | ||
| "RiskAnalysis", | ||
| ] |
There was a problem hiding this comment.
The analysis modules in this directory (quote_analysis, bottleneck_analysis, etc.) are not imported or used anywhere in the application. The logic seems to have been duplicated in backend/app/services/engineering/tools.py. These modules are currently dead code. They should be used by EngineeringToolsService to avoid code duplication.
| return "\n\n".join( | ||
| item.to_text() | ||
| for item in self.tools.interview_project_team( | ||
| self.graph_id, params.get("topics", []), int(params.get("limit", 5)) |
There was a problem hiding this comment.
There is a type mismatch when calling self.tools.interview_project_team. You are passing params.get("topics", []) which is a list, but the method expects a string for its query parameter. This will likely cause a TypeError or unexpected behavior at runtime. You should probably join the topics list into a string to form a valid query.
Suggested change
| self.graph_id, params.get("topics", []), int(params.get("limit", 5)) | |
| self.graph_id, " ".join(params.get("topics", [])), int(params.get("limit", 5)) |
| ): | ||
| outcome = "positive" | ||
| elif any( | ||
| k in text_lower for k in ["rejected", "failed", "conflict", "rejected"] |
There was a problem hiding this comment.
The list of keywords for detecting a negative outcome contains a duplicate "rejected".
Suggested change
| k in text_lower for k in ["rejected", "failed", "conflict", "rejected"] | |
| k in text_lower for k in ["rejected", "failed", "conflict"] |
Comment on lines
+340
to
+348
| critical_indicators = [ | ||
| "critical path", | ||
| "blocking", | ||
| "essential", | ||
| "mandatory", | ||
| "blocking", | ||
| "serial", | ||
| "sequential", | ||
| ] |
There was a problem hiding this comment.
The critical_indicators list contains a duplicate entry for "blocking". Removing the duplicate will make the list cleaner and avoid a redundant check.
critical_indicators = [
"critical path",
"blocking",
"essential",
"mandatory",
"serial",
"sequential",
]
Comment on lines
+210
to
+230
| stopwords = { | ||
| "this", | ||
| "that", | ||
| "with", | ||
| "from", | ||
| "have", | ||
| "been", | ||
| "were", | ||
| "they", | ||
| "their", | ||
| "would", | ||
| "could", | ||
| "should", | ||
| "there", | ||
| "which", | ||
| "what", | ||
| "when", | ||
| "where", | ||
| "been", | ||
| "have", | ||
| } |
There was a problem hiding this comment.
The stopwords set contains duplicate entries for "been" and "have". While a set handles duplicates automatically, removing them improves code clarity and maintainability.
stopwords = {
"this",
"that",
"with",
"from",
"have",
"been",
"were",
"they",
"their",
"would",
"could",
"should",
"there",
"which",
"what",
"when",
"where",
}
Comment on lines
+151
to
+164
| quote_markers = [ | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| '"', | ||
| """, """, | ||
| "「", | ||
| "」", | ||
| "『", | ||
| "』", | ||
| '"', | ||
| ] |
There was a problem hiding this comment.
The quote_markers list is difficult to read, contains many duplicate " characters, and is missing single quotes which seem to be intended. Using a set of unique characters would be cleaner and more efficient.
quote_markers = {'"', "'", "「", "」", "『", "』"}| "blocking", | ||
| "essential", | ||
| "mandatory", | ||
| "blocking", |
There was a problem hiding this comment.
The keyword "blocking" is duplicated in the critical_indicators list. Removing this redundant entry will make the code cleaner.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/engineering/analysis/bottleneck_analysis.py`:
- Around line 251-260: _extract_components currently only returns source/target
names so it almost always returns <=2 items, which makes critical-path detection
fire unconditionally; update _extract_components to first look for real
path/dependency data (e.g. keys like "path", "dependency_chain", "dependencies",
or "nodes") and extract up to three unique node/component names in order from
that list, preserving original order and skipping duplicates, and only if none
of those exist fall back to the existing source_node_name/target_node_name
logic; apply the same change to the other implementation at the similar location
(the other _extract_components occurrence referenced in the review) so
critical-path is based on actual path/dependency data rather than just edge
endpoints.
In `@backend/app/services/engineering/analysis/collaboration_analysis.py`:
- Around line 78-104: The current loop uses seen_types to skip subsequent edges
of the same collab_type, so the code only preserves the first match's
participants, examples, and metrics; instead build an aggregator keyed by
collab_type (e.g., a dict mapping collab_type -> aggregate state), and for each
fact/edge that passes _indicates_collaboration and _classify_collaboration
update that entry by merging participants from _extract_participants, appending
examples (from fact), accumulating counts or lists needed to recompute
consultation_frequency and review_effectiveness (using
_compute_consultation_frequency and _compute_review_effectiveness or by storing
raw inputs to compute them later), and merging effectiveness assessments from
_assess_effectiveness; after processing all edges convert each aggregated entry
into a CollaborationAnalysisResultModel (calling _describe and
_suggest_improvements as needed) and append to collaborations instead of
skipping duplicates via seen_types.
In `@backend/app/services/engineering/analysis/design_quality.py`:
- Around line 116-118: The exception handler in DesignQualityAnalysis.analyze
currently returns an empty list on failure, which differs from the non-error
path that returns _generate_default_assessments(limit); change the except block
to return _generate_default_assessments(limit) instead of [] and ensure the
caught exception (variable e) is still logged via logger.error so failures and
empty search results yield the same fail-safe default; update the handler in the
DesignQualityAnalysis.analyze method where the except Exception as e block is
defined.
In `@backend/app/services/engineering/analysis/risk_analysis.py`:
- Around line 274-306: The current likelihood matching uses substring checks (if
k in fact_lower), causing false positives like "uncertain" matching "certain";
update the logic around high_likelihood/low_likelihood to use token or
word-boundary matching instead (e.g., split fact_lower into word tokens and
check membership against a set, or use regex with r'\b<term>\b' and re.escape
for each k) when computing high_count and low_count; adjust the computations
that produce high_count and low_count (the lines referencing high_count =
sum(...) and low_count = sum(...), and the variables high_likelihood,
low_likelihood, fact_lower) accordingly and ensure any needed import (re) is
added.
In `@backend/app/services/engineering/report_agent.py`:
- Around line 176-187: The local variables team_interviews and
scenario_comparisons are computed but never attached to the EngineeringReport,
so either persist them into the report (e.g., add as sections or include in the
appendix via the EngineeringReport instance) or remove the calls entirely to
avoid wasted runtime; specifically either take the results from
self.tools.interview_project_team(graph_id=self.graph_id, limit=10) and
self.tools.compare_scenario_outcomes(graph_id=self.graph_id, limit=5) and attach
them to the EngineeringReport object (or render them via the same
section/appendix code used for other phases) and keep/update the
progress_callback calls, or delete those calls and their progress_callback
update to eliminate the unused work. Ensure any references to graph_id and
progress_callback are updated accordingly.
- Around line 757-763: The branch handling tool_name == "interview_project_team"
is passing params.get("topics", []) (a list) into
self.tools.interview_project_team which expects query: str; convert the topics
list into a single string (e.g., join topics with commas or spaces, or use the
first topic if that fits the domain) before calling
interview_project_team(self.graph_id, query, limit) so storage.search gets a
string query; keep the existing int(params.get("limit", 5)) usage for limit and
ensure you call self.tools.interview_project_team with the graph_id, the
constructed string query, and the limit.
In `@backend/app/services/engineering/report_models.py`:
- Around line 441-467: EngineeringReport currently lacks storage and
serialization for team_interviews and scenario_comparisons computed in
report_agent.py; add two new dataclass fields (e.g., team_interviews:
List[TeamInterviewResult] = field(default_factory=list) and
scenario_comparisons: List[ScenarioComparisonResult] =
field(default_factory=list)) to the EngineeringReport model and update its
to_dict method to include "team_interviews": [t.to_dict() for t in
self.team_interviews] and "scenario_comparisons": [s.to_dict() for s in
self.scenario_comparisons]; also import or define the TeamInterviewResult and
ScenarioComparisonResult types (or use appropriate existing classes) so the
fields serialize correctly and have sensible default_factory values.
- Around line 119-133: The trailing metrics line currently always formats as
f"{util_info} | {wait_info}" which can emit stray pipes when one side is empty;
inside the same method (to_text / the block that computes util_info and
wait_info) replace that final f-string with logic that builds a list of
non-empty parts (check util_info and wait_info), join them with " | " and only
append the resulting string if the list is non-empty so the output never
contains leading/trailing or solitary "|" characters; use the existing variables
util_info and wait_info and return the joined string (or empty string) in place
of the current f"{util_info} | {wait_info}".
In `@backend/app/services/engineering/tools.py`:
- Around line 630-656: The current loop uses seen_types and continues after
seen_types.add(collab_type), which causes later edges of the same collaboration
type to be ignored and makes metrics like consultation_frequency,
review_effectiveness, and examples order-dependent; change the logic to
aggregate per collaboration type instead: create a dict keyed by collab_type
that accumulates occurrences (edges), facts/examples, participant sets (from
_extract_participants), and any raw inputs needed by
_compute_consultation_frequency and _compute_review_effectiveness while
iterating through all edges (use _indicates_collaboration and
_classify_collaboration to bucket entries), then after the loop compute the
final consultation_frequency and review_effectiveness for each collab_type and
build one CollaborationAnalysis per bucket (calling _suggest_improvements,
_assess_effectiveness, etc. once per aggregated bucket) instead of skipping
later matches.
- Around line 475-485: _extract_components currently only returns source/target
for an edge causing len(components)<=2 and triggering unconditional
"system-wide" critical-path logic; change it to derive components from real
topology by extracting intermediate nodes or path data from the edge (e.g.,
check keys like "path", "nodes", "intermediate_nodes" or traverse edge["trace"]
if present) and collect up to three distinct component names, falling back to
keyword matching only when no topology data exists; update any call sites that
expect just endpoints (references: _extract_components) so the critical-path
decision logic uses the richer component list rather than endpoint count alone.
- Around line 182-192: search_edges() currently returns only relationship props
so edges lack source_node_name/target_node_name and downstream helpers like
_extract_speaker, _extract_affected_components, _extract_participants,
_extract_affected_stakeholders, and _extract_agent_info get "Unknown Agent";
update search_edges() to enrich each edge with the node name fields the storage
contract expects (add source_node_name = record.get("src_name","") and
target_node_name = record.get("tgt_name","") or equivalent keys used in
get_graph_data()), or explicitly document/emit a different schema for search
results—ensure the enrichment uses the same keys as get_graph_data() so
_extract_speaker and the other helpers can rely on them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 814b94d7-01a7-4d1e-b757-277da1f1b390
📒 Files selected for processing (12)
backend/app/services/__init__.pybackend/app/services/engineering/__init__.pybackend/app/services/engineering/analysis/__init__.pybackend/app/services/engineering/analysis/bottleneck_analysis.pybackend/app/services/engineering/analysis/collaboration_analysis.pybackend/app/services/engineering/analysis/design_quality.pybackend/app/services/engineering/analysis/quote_analysis.pybackend/app/services/engineering/analysis/risk_analysis.pybackend/app/services/engineering/report_agent.pybackend/app/services/engineering/report_models.pybackend/app/services/engineering/tools.pybackend/app/services/report_agent.py
Comment on lines
+251
to
+260
| def _extract_components(self, edge: Dict[str, Any]) -> List[str]: | ||
| components = [] | ||
| if isinstance(edge, dict): | ||
| source = edge.get("source_node_name", "") | ||
| target = edge.get("target_node_name", "") | ||
| if source: | ||
| components.append(source) | ||
| if target: | ||
| components.append(target) | ||
| return components[:3] |
There was a problem hiding this comment.
Critical-path detection is effectively unconditional.
_extract_components() can only yield the edge's source/target endpoints, so len(components) <= 2 is true for nearly every record. That makes almost every bottleneck look system-wide and removes the value of the critical-path metric.
🛠️ Suggested fix
- on_critical = (
- any(k in text_lower for k in critical_indicators) or len(components) <= 2
- )
+ on_critical = any(k in text_lower for k in critical_indicators)If you need a fallback heuristic, derive it from actual dependency/path data rather than the number of endpoints on one edge.
Also applies to: 349-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/analysis/bottleneck_analysis.py` around
lines 251 - 260, _extract_components currently only returns source/target names
so it almost always returns <=2 items, which makes critical-path detection fire
unconditionally; update _extract_components to first look for real
path/dependency data (e.g. keys like "path", "dependency_chain", "dependencies",
or "nodes") and extract up to three unique node/component names in order from
that list, preserving original order and skipping duplicates, and only if none
of those exist fall back to the existing source_node_name/target_node_name
logic; apply the same change to the other implementation at the similar location
(the other _extract_components occurrence referenced in the review) so
critical-path is based on actual path/dependency data rather than just edge
endpoints.
Comment on lines
+78
to
+104
| if self._indicates_collaboration(fact): | ||
| collab_type = self._classify_collaboration(fact) | ||
| if collab_type in seen_types: | ||
| continue | ||
| seen_types.add(collab_type) | ||
|
|
||
| participants = self._extract_participants(edge) | ||
| examples = [fact] if fact else [] | ||
| effectiveness = self._assess_effectiveness(fact) | ||
|
|
||
| # Compute collaboration metrics | ||
| consultation_freq = self._compute_consultation_frequency(edge, fact) | ||
| review_effectiveness = self._compute_review_effectiveness(fact) | ||
|
|
||
| collaborations.append( | ||
| CollaborationAnalysisResultModel( | ||
| collaboration_type=collab_type, | ||
| participants=participants, | ||
| description=self._describe(fact), | ||
| effectiveness=effectiveness, | ||
| examples=examples, | ||
| improvement_suggestions=self._suggest_improvements( | ||
| collab_type | ||
| ), | ||
| consultation_frequency=consultation_freq, | ||
| review_effectiveness=review_effectiveness, | ||
| ) |
There was a problem hiding this comment.
Aggregate same-type collaborations instead of keeping the first match.
Once collab_type is added to seen_types, every later edge of that type is skipped. The returned participants, examples, consultation frequency, and review effectiveness therefore describe one arbitrary edge, not the overall collaboration pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/analysis/collaboration_analysis.py` around
lines 78 - 104, The current loop uses seen_types to skip subsequent edges of the
same collab_type, so the code only preserves the first match's participants,
examples, and metrics; instead build an aggregator keyed by collab_type (e.g., a
dict mapping collab_type -> aggregate state), and for each fact/edge that passes
_indicates_collaboration and _classify_collaboration update that entry by
merging participants from _extract_participants, appending examples (from fact),
accumulating counts or lists needed to recompute consultation_frequency and
review_effectiveness (using _compute_consultation_frequency and
_compute_review_effectiveness or by storing raw inputs to compute them later),
and merging effectiveness assessments from _assess_effectiveness; after
processing all edges convert each aggregated entry into a
CollaborationAnalysisResultModel (calling _describe and _suggest_improvements as
needed) and append to collaborations instead of skipping duplicates via
seen_types.
Comment on lines
+116
to
+118
| except Exception as e: | ||
| logger.error(f"DesignQualityAnalysis.analyze failed: {e}") | ||
| return [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/app/services/engineering/analysis/design_quality.py | head -150Repository: lilubot/MiroFish-Offline
Length of output: 6090
🏁 Script executed:
cat -n backend/app/services/engineering/analysis/design_quality.py | sed -n '150,250p'Repository: lilubot/MiroFish-Offline
Length of output: 3842
🏁 Script executed:
cat -n backend/app/services/engineering/analysis/design_quality.py | sed -n '250,350p'Repository: lilubot/MiroFish-Offline
Length of output: 4140
🏁 Script executed:
cat -n backend/app/services/engineering/analysis/design_quality.py | sed -n '350,450p'Repository: lilubot/MiroFish-Offline
Length of output: 4227
🏁 Script executed:
cat -n backend/app/services/engineering/analysis/design_quality.py | sed -n '450,550p'Repository: lilubot/MiroFish-Offline
Length of output: 3362
Return the same fail-safe default for both storage failures and empty search results.
The exception handler at lines 116–118 returns [], but when storage.search() succeeds with no findings, line 111 returns _generate_default_assessments(limit) (1–2 default assessments). This inconsistency masks storage/schema/runtime failures: a transient storage.search() error now looks identical to a genuinely empty graph, preventing proper error reporting and observability.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 116-116: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/analysis/design_quality.py` around lines 116
- 118, The exception handler in DesignQualityAnalysis.analyze currently returns
an empty list on failure, which differs from the non-error path that returns
_generate_default_assessments(limit); change the except block to return
_generate_default_assessments(limit) instead of [] and ensure the caught
exception (variable e) is still logged via logger.error so failures and empty
search results yield the same fail-safe default; update the handler in the
DesignQualityAnalysis.analyze method where the except Exception as e block is
defined.
Comment on lines
+274
to
+306
| high_likelihood = [ | ||
| "likely", | ||
| "probable", | ||
| "certain", | ||
| "known", | ||
| "frequent", | ||
| "consistent", | ||
| "regular", | ||
| "common", | ||
| "always", | ||
| "certainly", | ||
| ] | ||
| low_likelihood = [ | ||
| "unlikely", | ||
| "rare", | ||
| "infrequent", | ||
| "occasional", | ||
| "seldom", | ||
| "sometimes", | ||
| "possibly", | ||
| "perhaps", | ||
| "maybe", | ||
| "uncertain", | ||
| ] | ||
|
|
||
| high_count = sum(1 for k in high_likelihood if k in fact_lower) | ||
| low_count = sum(1 for k in low_likelihood if k in fact_lower) | ||
|
|
||
| if high_count > low_count: | ||
| return "high" | ||
| elif low_count > high_count: | ||
| return "low" | ||
| return "medium" |
There was a problem hiding this comment.
Use token or word-boundary matching for likelihood terms.
if k in fact_lower makes "certain" match "uncertain" and "known" match "unknown", so obviously low-confidence descriptions can still increment the high-likelihood bucket. That skew propagates directly into risk_level.
🎯 Safer keyword matching
- high_count = sum(1 for k in high_likelihood if k in fact_lower)
- low_count = sum(1 for k in low_likelihood if k in fact_lower)
+ tokens = set(re.findall(r"\b[a-z]+\b", fact_lower))
+ high_count = sum(1 for k in high_likelihood if k in tokens)
+ low_count = sum(1 for k in low_likelihood if k in tokens)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/analysis/risk_analysis.py` around lines 274
- 306, The current likelihood matching uses substring checks (if k in
fact_lower), causing false positives like "uncertain" matching "certain"; update
the logic around high_likelihood/low_likelihood to use token or word-boundary
matching instead (e.g., split fact_lower into word tokens and check membership
against a set, or use regex with r'\b<term>\b' and re.escape for each k) when
computing high_count and low_count; adjust the computations that produce
high_count and low_count (the lines referencing high_count = sum(...) and
low_count = sum(...), and the variables high_likelihood, low_likelihood,
fact_lower) accordingly and ensure any needed import (re) is added.
Comment on lines
+176
to
+187
| # Phase 6: Team Interviews (stored but not as separate section) | ||
| team_interviews = self.tools.interview_project_team( | ||
| graph_id=self.graph_id, limit=10 | ||
| ) | ||
|
|
||
| if progress_callback: | ||
| progress_callback("analyzing", 85, "Comparing scenario outcomes...") | ||
|
|
||
| # Phase 7: Scenario Comparison (stored but not as separate section) | ||
| scenario_comparisons = self.tools.compare_scenario_outcomes( | ||
| graph_id=self.graph_id, limit=5 | ||
| ) |
There was a problem hiding this comment.
Persist or remove these two analysis phases.
team_interviews and scenario_comparisons are assigned to locals only. They never get attached to EngineeringReport, rendered into sections, or included in the appendix, so the completed report silently omits two analyses while still paying their runtime cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/report_agent.py` around lines 176 - 187, The
local variables team_interviews and scenario_comparisons are computed but never
attached to the EngineeringReport, so either persist them into the report (e.g.,
add as sections or include in the appendix via the EngineeringReport instance)
or remove the calls entirely to avoid wasted runtime; specifically either take
the results from self.tools.interview_project_team(graph_id=self.graph_id,
limit=10) and self.tools.compare_scenario_outcomes(graph_id=self.graph_id,
limit=5) and attach them to the EngineeringReport object (or render them via the
same section/appendix code used for other phases) and keep/update the
progress_callback calls, or delete those calls and their progress_callback
update to eliminate the unused work. Ensure any references to graph_id and
progress_callback are updated accordingly.
Comment on lines
+119
to
+133
| util_info = ( | ||
| f"Utilization: {self.workstation_utilization:.1%}" | ||
| if self.workstation_utilization | ||
| else "" | ||
| ) | ||
| wait_info = "" | ||
| if self.wait_times and self.wait_times.get("has_wait_time"): | ||
| wait_info = f"Wait: {self.wait_times.get('estimated_delay', 'N/A')} ({self.wait_times.get('severity', 'N/A')} severity)" | ||
| return ( | ||
| f"Bottleneck: {self.bottleneck_name} [{self.severity.upper()}]\n" | ||
| f"Description: {self.description}\n" | ||
| f"Affected Components: {components}\n" | ||
| f"Evidence:\n - {evidence}\n" | ||
| f"Recommendation: {self.recommendation}\n" | ||
| f"{util_info} | {wait_info}".strip() |
There was a problem hiding this comment.
Build the trailing metrics line from non-empty parts only.
The fixed f"{util_info} | {wait_info}" emits |, util |, or | wait ... whenever one side is empty, so to_text() can produce malformed output for normal cases.
🧹 Suggested fix
+ details = " | ".join(part for part in [util_info, wait_info] if part)
return (
f"Bottleneck: {self.bottleneck_name} [{self.severity.upper()}]\n"
f"Description: {self.description}\n"
f"Affected Components: {components}\n"
f"Evidence:\n - {evidence}\n"
- f"Recommendation: {self.recommendation}\n"
- f"{util_info} | {wait_info}".strip()
+ f"Recommendation: {self.recommendation}"
+ + (f"\n{details}" if details else "")
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/report_models.py` around lines 119 - 133,
The trailing metrics line currently always formats as f"{util_info} |
{wait_info}" which can emit stray pipes when one side is empty; inside the same
method (to_text / the block that computes util_info and wait_info) replace that
final f-string with logic that builds a list of non-empty parts (check util_info
and wait_info), join them with " | " and only append the resulting string if the
list is non-empty so the output never contains leading/trailing or solitary "|"
characters; use the existing variables util_info and wait_info and return the
joined string (or empty string) in place of the current f"{util_info} |
{wait_info}".
Comment on lines
+441
to
+467
| # Analysis results stored for structured access | ||
| quote_analysis: List[QuoteAccuracyResult] = field(default_factory=list) | ||
| bottleneck_analysis: List[BottleneckAnalysis] = field(default_factory=list) | ||
| collaboration_analysis: List[CollaborationAnalysis] = field(default_factory=list) | ||
| design_quality: List[DesignQualityResult] = field(default_factory=list) | ||
| risk_analysis: List[RiskPrediction] = field(default_factory=list) | ||
|
|
||
| def to_dict(self) -> Dict[str, Any]: | ||
| return { | ||
| "report_id": self.report_id, | ||
| "simulation_id": self.simulation_id, | ||
| "graph_id": self.graph_id, | ||
| "title": self.title, | ||
| "summary": self.summary, | ||
| "status": self.status.value, | ||
| "sections": [s.to_dict() for s in self.sections], | ||
| "markdown_content": self.markdown_content, | ||
| "created_at": self.created_at, | ||
| "completed_at": self.completed_at, | ||
| "error": self.error, | ||
| "analysis_results": { | ||
| "quotes": [q.to_dict() for q in self.quote_analysis], | ||
| "bottlenecks": [b.to_dict() for b in self.bottleneck_analysis], | ||
| "collaboration": [c.to_dict() for c in self.collaboration_analysis], | ||
| "design_quality": [d.to_dict() for d in self.design_quality], | ||
| "risks": [r.to_dict() for r in self.risk_analysis], | ||
| }, |
There was a problem hiding this comment.
Persist the interview and scenario-comparison results on EngineeringReport.
EngineeringReport only has slots for quotes, bottlenecks, collaboration, design quality, and risks, but backend/app/services/engineering/report_agent.py:119-197 also computes team_interviews and scenario_comparisons. Those two analysis phases are currently dropped from the saved report because there is nowhere to store or serialize them.
🧩 Minimal shape update
quote_analysis: List[QuoteAccuracyResult] = field(default_factory=list)
bottleneck_analysis: List[BottleneckAnalysis] = field(default_factory=list)
collaboration_analysis: List[CollaborationAnalysis] = field(default_factory=list)
design_quality: List[DesignQualityResult] = field(default_factory=list)
risk_analysis: List[RiskPrediction] = field(default_factory=list)
+ team_interviews: List[TeamInterviewResult] = field(default_factory=list)
+ scenario_comparisons: List[ScenarioComparisonResult] = field(
+ default_factory=list
+ )
def to_dict(self) -> Dict[str, Any]:
return {
"report_id": self.report_id,
"simulation_id": self.simulation_id,
@@
"analysis_results": {
"quotes": [q.to_dict() for q in self.quote_analysis],
"bottlenecks": [b.to_dict() for b in self.bottleneck_analysis],
"collaboration": [c.to_dict() for c in self.collaboration_analysis],
"design_quality": [d.to_dict() for d in self.design_quality],
"risks": [r.to_dict() for r in self.risk_analysis],
+ "team_interviews": [t.to_dict() for t in self.team_interviews],
+ "scenario_comparisons": [
+ s.to_dict() for s in self.scenario_comparisons
+ ],
},
}📝 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.
Suggested change
| # Analysis results stored for structured access | |
| quote_analysis: List[QuoteAccuracyResult] = field(default_factory=list) | |
| bottleneck_analysis: List[BottleneckAnalysis] = field(default_factory=list) | |
| collaboration_analysis: List[CollaborationAnalysis] = field(default_factory=list) | |
| design_quality: List[DesignQualityResult] = field(default_factory=list) | |
| risk_analysis: List[RiskPrediction] = field(default_factory=list) | |
| def to_dict(self) -> Dict[str, Any]: | |
| return { | |
| "report_id": self.report_id, | |
| "simulation_id": self.simulation_id, | |
| "graph_id": self.graph_id, | |
| "title": self.title, | |
| "summary": self.summary, | |
| "status": self.status.value, | |
| "sections": [s.to_dict() for s in self.sections], | |
| "markdown_content": self.markdown_content, | |
| "created_at": self.created_at, | |
| "completed_at": self.completed_at, | |
| "error": self.error, | |
| "analysis_results": { | |
| "quotes": [q.to_dict() for q in self.quote_analysis], | |
| "bottlenecks": [b.to_dict() for b in self.bottleneck_analysis], | |
| "collaboration": [c.to_dict() for c in self.collaboration_analysis], | |
| "design_quality": [d.to_dict() for d in self.design_quality], | |
| "risks": [r.to_dict() for r in self.risk_analysis], | |
| }, | |
| # Analysis results stored for structured access | |
| quote_analysis: List[QuoteAccuracyResult] = field(default_factory=list) | |
| bottleneck_analysis: List[BottleneckAnalysis] = field(default_factory=list) | |
| collaboration_analysis: List[CollaborationAnalysis] = field(default_factory=list) | |
| design_quality: List[DesignQualityResult] = field(default_factory=list) | |
| risk_analysis: List[RiskPrediction] = field(default_factory=list) | |
| team_interviews: List[TeamInterviewResult] = field(default_factory=list) | |
| scenario_comparisons: List[ScenarioComparisonResult] = field( | |
| default_factory=list | |
| ) | |
| def to_dict(self) -> Dict[str, Any]: | |
| return { | |
| "report_id": self.report_id, | |
| "simulation_id": self.simulation_id, | |
| "graph_id": self.graph_id, | |
| "title": self.title, | |
| "summary": self.summary, | |
| "status": self.status.value, | |
| "sections": [s.to_dict() for s in self.sections], | |
| "markdown_content": self.markdown_content, | |
| "created_at": self.created_at, | |
| "completed_at": self.completed_at, | |
| "error": self.error, | |
| "analysis_results": { | |
| "quotes": [q.to_dict() for q in self.quote_analysis], | |
| "bottlenecks": [b.to_dict() for b in self.bottleneck_analysis], | |
| "collaboration": [c.to_dict() for c in self.collaboration_analysis], | |
| "design_quality": [d.to_dict() for d in self.design_quality], | |
| "risks": [r.to_dict() for r in self.risk_analysis], | |
| "team_interviews": [t.to_dict() for t in self.team_interviews], | |
| "scenario_comparisons": [ | |
| s.to_dict() for s in self.scenario_comparisons | |
| ], | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/report_models.py` around lines 441 - 467,
EngineeringReport currently lacks storage and serialization for team_interviews
and scenario_comparisons computed in report_agent.py; add two new dataclass
fields (e.g., team_interviews: List[TeamInterviewResult] =
field(default_factory=list) and scenario_comparisons:
List[ScenarioComparisonResult] = field(default_factory=list)) to the
EngineeringReport model and update its to_dict method to include
"team_interviews": [t.to_dict() for t in self.team_interviews] and
"scenario_comparisons": [s.to_dict() for s in self.scenario_comparisons]; also
import or define the TeamInterviewResult and ScenarioComparisonResult types (or
use appropriate existing classes) so the fields serialize correctly and have
sensible default_factory values.
Comment on lines
+182
to
+192
| def _extract_speaker(self, edge: Dict[str, Any]) -> tuple: | ||
| """Extract speaker name and role from edge data.""" | ||
| source_name = "" | ||
| target_name = "" | ||
| if isinstance(edge, dict): | ||
| source_name = edge.get("source_node_name", "") | ||
| target_name = edge.get("target_node_name", "") | ||
|
|
||
| speaker = source_name or target_name or "Unknown Agent" | ||
| speaker_role = self._infer_role(speaker) | ||
| return speaker, speaker_role |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether storage/query code ever populates these fields.
rg -n -C3 'source_node_name|target_node_name' backend/app/storage backend/app/services/engineering
# 2) Inspect how edge search results are materialized.
rg -n -C4 'dict\(record\["r"\]\)|search_edges\(' backend/app/storage/search_service.py backend/app/storage/neo4j_storage.pyRepository: lilubot/MiroFish-Offline
Length of output: 11544
The search_edges() path does not enrich edges with source_node_name and target_node_name, causing silent data loss.
In backend/app/storage/search_service.py, the search_edges() method returns edges as {**dict(record["r"]), "uuid": record["r"]["uuid"], "_score": record["score"]} (lines 136, 157). This unpacks only relationship properties—not the node name fields that the storage contract promises.
In contrast, get_graph_data() in backend/app/storage/neo4j_storage.py correctly enriches edges with:
ed["source_node_name"] = record["src_name"] or ""
ed["target_node_name"] = record["tgt_name"] or ""Multiple helper methods across the engineering service depend on these fields (_extract_speaker, _extract_affected_components, _extract_participants, _extract_affected_stakeholders, _extract_agent_info). When edges come from search results, these fields are missing, causing fallback to "Unknown Agent" and loss of actor attribution.
Apply the same node name enrichment pattern to search_edges() to maintain the edge format contract, or restrict search results to relationship properties only and document the schema difference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/tools.py` around lines 182 - 192,
search_edges() currently returns only relationship props so edges lack
source_node_name/target_node_name and downstream helpers like _extract_speaker,
_extract_affected_components, _extract_participants,
_extract_affected_stakeholders, and _extract_agent_info get "Unknown Agent";
update search_edges() to enrich each edge with the node name fields the storage
contract expects (add source_node_name = record.get("src_name","") and
target_node_name = record.get("tgt_name","") or equivalent keys used in
get_graph_data()), or explicitly document/emit a different schema for search
results—ensure the enrichment uses the same keys as get_graph_data() so
_extract_speaker and the other helpers can rely on them.
Comment on lines
+475
to
+485
| def _extract_components(self, edge: Dict[str, Any]) -> List[str]: | ||
| """Extract affected components from edge data.""" | ||
| components = [] | ||
| if isinstance(edge, dict): | ||
| source = edge.get("source_node_name", "") | ||
| target = edge.get("target_node_name", "") | ||
| if source: | ||
| components.append(source) | ||
| if target: | ||
| components.append(target) | ||
| return components[:3] |
There was a problem hiding this comment.
Critical-path detection is effectively unconditional here as well.
_extract_components() only returns the source/target endpoints of a single edge, so len(components) <= 2 is almost always true. Most bottlenecks will therefore be reported as system-wide blockers regardless of the actual path.
🛠️ Suggested fix
- on_critical = (
- any(k in text_lower for k in critical_indicators) or len(components) <= 2
- )
+ on_critical = any(k in text_lower for k in critical_indicators)If you want a fallback beyond keyword matching, base it on real graph topology rather than endpoint count.
Also applies to: 551-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/tools.py` around lines 475 - 485,
_extract_components currently only returns source/target for an edge causing
len(components)<=2 and triggering unconditional "system-wide" critical-path
logic; change it to derive components from real topology by extracting
intermediate nodes or path data from the edge (e.g., check keys like "path",
"nodes", "intermediate_nodes" or traverse edge["trace"] if present) and collect
up to three distinct component names, falling back to keyword matching only when
no topology data exists; update any call sites that expect just endpoints
(references: _extract_components) so the critical-path decision logic uses the
richer component list rather than endpoint count alone.
Comment on lines
+630
to
+656
| if self._indicates_collaboration(fact): | ||
| collab_type = self._classify_collaboration(fact) | ||
| if collab_type in seen_types: | ||
| continue | ||
| seen_types.add(collab_type) | ||
|
|
||
| participants = self._extract_participants(edge) | ||
| examples = [fact] if fact else [] | ||
| effectiveness = self._assess_effectiveness(fact) | ||
|
|
||
| # Compute collaboration metrics | ||
| consultation_freq = self._compute_consultation_frequency(edge, fact) | ||
| review_effectiveness = self._compute_review_effectiveness(fact) | ||
|
|
||
| collaborations.append( | ||
| CollaborationAnalysis( | ||
| collaboration_type=collab_type, | ||
| participants=participants, | ||
| description=fact[:200] if len(fact) > 200 else fact, | ||
| effectiveness=effectiveness, | ||
| examples=examples, | ||
| improvement_suggestions=self._suggest_improvements( | ||
| collab_type | ||
| ), | ||
| consultation_frequency=consultation_freq, | ||
| review_effectiveness=review_effectiveness, | ||
| ) |
There was a problem hiding this comment.
This scorer discards later matches of the same collaboration type.
After seen_types.add(collab_type), later edges of that type are ignored. That makes consultation_frequency, review_effectiveness, and examples depend on search ordering instead of the full set of matching interactions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/engineering/tools.py` around lines 630 - 656, The
current loop uses seen_types and continues after seen_types.add(collab_type),
which causes later edges of the same collaboration type to be ignored and makes
metrics like consultation_frequency, review_effectiveness, and examples
order-dependent; change the logic to aggregate per collaboration type instead:
create a dict keyed by collab_type that accumulates occurrences (edges),
facts/examples, participant sets (from _extract_participants), and any raw
inputs needed by _compute_consultation_frequency and
_compute_review_effectiveness while iterating through all edges (use
_indicates_collaboration and _classify_collaboration to bucket entries), then
after the loop compute the final consultation_frequency and review_effectiveness
for each collab_type and build one CollaborationAnalysis per bucket (calling
_suggest_improvements, _assess_effectiveness, etc. once per aggregated bucket)
instead of skipping later matches.
Implement complete agent-based simulation system that transforms MiroFish from social media simulation into universal manufacturing disruption prediction platform. Core Components: - Entity Mapper: Maps scheduling entities (machines, operators, jobs) to OASIS agent profiles with domain-specific behaviors - State Manager: Tracks real-time factory state via database polling - Disruption Engine: Agent-based simulation predicting machine breakdowns, operator absence, rush orders with confidence scores - Prediction Bridge: Feeds simulation results to scheduler for proactive rescheduling - Database Integration: PostgreSQL adapters for ERP/MES/SCADA connectivity - REST API: Shop system integration endpoints for data ingestion and prediction retrieval Job Shop Scenario: - Machine agents with MTBF-based failure prediction - Operator agents with shift-based availability modeling - Job agents with priority-driven urgency simulation - Integration with OR-Tools CP-SAT solver for schedule optimization Architecture: - Plugin-based scenario system for future expansion (supply chain, workforce, etc.) - Universal REST API abstracting simulation complexity - Database polling from live ERP/MES systems Documentation: - STRATEGIC_PLAN.md: Complete vision, roadmap, and success metrics - ARCHITECTURE_OPTIONS.md: Architecture decision documentation - Integration guide with usage examples Constraint: Maintain compatibility with existing MiroFish OASIS simulation framework while abstracting domain-specific concepts. Confidence: high Scope-risk: moderate Not-tested: Production deployment, real ERP database integration
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
ARCHITECTURE_OPTIONS.md-285-323 (1)
285-323: 🛠️ Refactor suggestion | 🟠 MajorMake the documented
Scenariocontract match the example plugin.
load_entities()/map_to_agents()are typed aroundList[Any], butJobShopScenario.load_entities()returns a keyed mapping andmap_to_agents()immediately treatsentitiesasentities["machines"]/entities["operators"]/entities["jobs"]. Right now the abstract interface and the example describe different APIs, which makes the plugin boundary ambiguous. Prefer a sharedEntitySet/mapping type here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE_OPTIONS.md` around lines 285 - 323, The abstract Scenario contract is inconsistent with the JobShopScenario example: load_entities and map_to_agents are typed as List[Any] but JobShopScenario.load_entities returns a keyed mapping accessed by map_to_agents (entities['machines'], etc.); update the interface to use a shared mapping type (e.g., EntitySet or Dict[str, List[Any>]) so the contract matches the plugin example. Concretely, change the abstract methods in Scenario—load_entities, map_to_agents, and any related docstrings—to accept/return the unified mapping type and update signatures/comments to reference EntitySet (or Dict[str, List[Any>]); ensure JobShopScenario keeps its implementation and that map_to_agents's parameter type matches the new EntitySet type.backend/app/services/scheduling/visualization.py-484-488 (1)
484-488:⚠️ Potential issue | 🟠 MajorThe queue-time metric is currently guaranteed to be zero.
e.start_time - e.end_timeis non-positive for every valid entry, sototal_wait_minutesnever reflects actual queueing and the bottleneck list collapses to utilization only. Compute wait from adjacent operations or another real ready-time signal instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/visualization.py` around lines 484 - 488, The current total_wait calculation uses (e.start_time - e.end_time) which is non-positive and always zeroes the queue metric; change the logic in the block that computes total_wait for machine_entries to compute gaps between consecutive operations instead: sort or iterate machine_entries in chronological order and for each entry compute wait = max(0, (current.start_time - previous.end_time).total_seconds() / 60) (or use a real ready_time if you have one on the entry), sum those waits into total_wait (or total_wait_minutes) so the queue-time reflects actual inter-operation queuing rather than e.start_time - e.end_time.backend/app/services/scheduling/visualization.py-187-205 (1)
187-205:⚠️ Potential issue | 🟠 MajorEither honor
window_hoursor remove it from the API.The function advertises a bounded heatmap, but the implementation always renders from the earliest entry to the latest entry and never reads the caller-supplied window. On long schedules this can generate much larger payloads than the signature suggests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/visualization.py` around lines 187 - 205, The generate_utilization_heatmap function currently ignores the window_hours parameter and always spans from the earliest entry to the latest; update it to honor window_hours by bounding the time range: compute raw_start = min(e.start_time ...) and raw_end = max(e.end_time ...), then set start_time = max(raw_start, raw_end - timedelta(hours=window_hours)) (and validate window_hours > 0), keep end_time = raw_end, and then build the hourly buckets using current = start_time with the existing loop over hours/current; ensure any downstream logic that uses hours, start_time or end_time uses the new bounded range.backend/app/api/digital_twin.py-230-232 (1)
230-232:⚠️ Potential issue | 🟠 MajorAvoid leaking backend exceptions through the API.
This returns
str(e)directly to callers and logs the same message verbatim. On this route especially, DB or mapping failures can include connection details, schema names, or other internal data from the submitted config. Return a generic client error and log sanitized context instead. The same response pattern repeats in the other handlers below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/digital_twin.py` around lines 230 - 232, The current except blocks that do "except Exception as e" (seen in the route handler containing the logger.error(f\"Failed to initialize: {e}\") and jsonify(... str(e) ...)) leak internal error details to callers and logs; change these handlers to return a generic error response (e.g. jsonify({"success": False, "error": "Internal server error"}) with a 500) and instead log a sanitized, non-sensitive message plus safe context (e.g. logger.exception or logger.error with a message like "Failed to initialize digital twin" and include only non-sensitive identifiers), and apply the same pattern to the other route handlers that currently return str(e) so internal DB/mapping details are not exposed.backend/app/services/scheduling/historical_data.py-448-456 (1)
448-456:⚠️ Potential issue | 🟠 MajorUse a real
job_typekey when querying historical aggregates.Both aggregate lookups are keyed by
HistoricalJobRecord.job_type, but these calls passjob.material. Unless those values happen to be identical, duration calibration and confidence scoring will miss the intended history and fall back to defaults.Also applies to: 472-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/historical_data.py` around lines 448 - 456, The code is using job.material when querying historical aggregates keyed by HistoricalJobRecord.job_type, causing misses; update the loader calls (e.g., get_average_job_duration and the confidence scoring call around job.historical_similar_job_duration and the block at lines ~472-474) to pass the real job type (job.job_type or the correct job_type property) instead of job.material so lookups use HistoricalJobRecord.job_type and return the intended aggregates.backend/app/services/digital_twin/db_integration.py-608-621 (1)
608-621:⚠️ Potential issue | 🟠 MajorKeep the first row per metric, not the last one seen.
get_machine_metrics()returns rows in descending timestamp order, but this loop overwritesavailability/performance/qualityon every match. The final OEE is built from the oldest row in the window instead of the newest one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/db_integration.py` around lines 608 - 621, The loop in get_machine_metrics (iterating over metrics) overwrites availability/performance/quality for every matching row, causing OEE to be computed from the oldest row; change the logic to keep the first (newest) value found per metric type by only assigning result["availability"], result["performance"], and result["quality"] if they are not already set (or are None/uninitialized), and stop early once all three metrics are populated (or break when result has all three keys) so the first encountered (most recent) row for each metric is preserved before computing result["oee"].backend/app/services/digital_twin/state_manager.py-785-798 (1)
785-798:⚠️ Potential issue | 🟠 Major
load_state()is still a no-op.After loading JSON, the method never reconstructs
_machines,_operators,_jobs, or_event_history, but it still returnsTrue. Callers will think state was restored when the manager is still empty.If you want, I can sketch the deserializers that rebuild the state objects from the saved snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/state_manager.py` around lines 785 - 798, load_state currently reads JSON but never rebuilds in-memory state; update load_state to deserialize the loaded JSON into actual MachineState, OperatorState, JobState (and any Event objects) and repopulate the manager's internal containers (_machines, _operators, _jobs, _event_history). Specifically, parse the expected keys from the JSON snapshot, call the appropriate constructors or classmethod deserializers on MachineState/OperatorState/JobState to recreate instances, replace/clear and set self._machines/self._operators/self._jobs (preserving keys/IDs), restore self._event_history as a list of event objects, validate required fields and raise/log errors on mismatch, and only return True when reconstruction succeeds (return False on validation/deserialization failure). Ensure load_state references the existing serialization format and uses the same field names the save routine emits.backend/app/services/scheduling/historical_data.py-398-405 (1)
398-405:⚠️ Potential issue | 🟠 MajorDatabase-backed operation calibration never matches any records.
calibrate_operation()filters onop_record["machine_type"], but_load_operations()only storesoperation_id,machine_id,planned_duration,actual_duration, andsetup_time. For database-loaded historysimilar_opsstays empty, so operation durations are never calibrated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/historical_data.py` around lines 398 - 405, calibrate_operation() is filtering op_record["machine_type"] but _load_operations() doesn’t populate machine_type (it only stores operation_id, machine_id, planned_duration, actual_duration, setup_time), so similar_ops stays empty; fix by modifying _load_operations() to include the machine_type (derive it from the machine_id or join the machines table) when building each op_record so that calibrate_operation() can correctly compare op_record["machine_type"] to machine_type.value, or alternatively update calibrate_operation() to map op_record["machine_id"] to the corresponding machine_type before comparison (use the same machine lookup used elsewhere in the loader/job_history code).backend/app/services/scheduling/solver.py-435-465 (1)
435-465:⚠️ Potential issue | 🟠 MajorTie
total_tardinessto the per-job tardiness terms.
total_tardinessis minimized but never constrained to equal the tardiness variables created inside the loop. CP-SAT can set it to0regardless of late jobs, so the tardiness weight currently has no effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/solver.py` around lines 435 - 465, total_tardiness is never linked to the per-job tardiness vars so the solver can ignore lateness; collect each per-job NewIntVar (created as tardiness_{job.job_id}) into a list (e.g., job_tardiness_vars) when you create them, then add a constraint tying total_tardiness to their sum using self.model.Add(total_tardiness == sum(job_tardiness_vars)); ensure you reference the same end_vars[(job.job_id, last_op.operation_id)] when creating each tardiness and keep total_tardiness bounds appropriate (it already is), then leave the existing objectives.append(total_tardiness * int(self.config.tardiness_weight * 1000)) as-is so the weight actually affects the objective.backend/app/services/scheduling/solver.py-166-198 (1)
166-198:⚠️ Potential issue | 🟠 MajorDowntime, changeovers, and labor never make it into the CP-SAT model.
Machine.status/maintenance_windowsare prepared but never translated into forbidden intervals,_add_setup_time_constraints()computes pairwise setup times without adding constraints, and_add_resource_constraints()is a stub. The solve therefore ignores unavailable machines, sequence-dependent setup, and operator constraints entirely.Also applies to: 346-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/solver.py` around lines 166 - 198, The solver currently prepares machine metadata (Machine.status, maintenance_windows) and computes setup times but never encodes them into the CP-SAT model: update _prepare_data/_prepare_model to translate maintenance_windows/Machine.status into forbidden intervals (add NoOverlap2/interval constraints or presence booleans per machine) so tasks cannot be scheduled during downtime, implement _add_setup_time_constraints to add precedence/transition constraints (sequence-dependent setup as transition times between interval variables for operations assigned to the same machine, using AddNoOverlap/NoOverlap2 with transition matrix or pairwise precedences), and fully implement _add_resource_constraints to model operator/labor capacity (resource capacity constraints or cumulative constraints linking operation intervals to resource demands). Use existing symbols machine_map, operation_list, _add_setup_time_constraints, _add_resource_constraints, and horizon to locate the spots to add forbidden-interval creation, interval-to-machine assignment, transition matrices for setup, and resource capacity enforcement so unavailable machines and sequence-dependent setups are respected by the CP-SAT model.backend/app/services/digital_twin/prediction_bridge.py-434-448 (1)
434-448:⚠️ Potential issue | 🟠 MajorModel temporary absences as time-bounded constraints.
The feedback carries
absence_timeandduration_minutes, but this updater ignores both and permanently setsshift_start = shift_end = 0. Every later solve will treat the operator as unavailable forever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/prediction_bridge.py` around lines 434 - 448, The _apply_operator_unavailability currently makes an operator permanently unavailable by setting operator.shift_start and operator.shift_end to 0; instead use the constraint's absence_time and duration_minutes to create a time-bounded unavailability window. Locate _apply_operator_unavailability and, when matching operator.operator_id, parse constraint["absence_time"] and constraint["duration_minutes"], compute the start and end timestamps (or time slots) for the absence, and either (a) add this interval to the operator's existing unavailability list/property (e.g., operator.unavailabilities.append(...) or operator.add_unavailability(...)) or (b) adjust shift_start/shift_end only for that interval rather than globally so future solves respect availability outside the absence window; ensure you do not overwrite permanent shift fields and that the code uses the SchedulingProblem/operator APIs to record time-bounded constraints.backend/app/services/scheduling/solver.py-817-821 (1)
817-821:⚠️ Potential issue | 🟠 MajorDon't compare makespan until the CP-SAT result is actually feasible.
JobShopSolver._build_schedule()returns a schedule withmakespan=0when the solve is infeasible or unknown. This branch will treat that as “better” than the heuristic result and return an unusable schedule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/solver.py` around lines 817 - 821, The current return logic compares optimized_schedule.makespan against heuristic_schedule.makespan even when the CP-SAT solve was infeasible/unknown (JobShopSolver._build_schedule() returns makespan=0 for those cases), causing an unusable schedule to be chosen; modify the logic to first detect whether the CP-SAT result is feasible/optimal (or that optimized_schedule is valid) before comparing makespans — e.g., check the solver result status or a validity flag produced by _build_schedule() for optimized_schedule and only fall back to comparing makespan if optimized_schedule is feasible/valid, otherwise return heuristic_schedule. Ensure you reference optimized_schedule, heuristic_schedule and JobShopSolver._build_schedule() when implementing the check.backend/app/services/digital_twin/entity_mapper.py-42-53 (1)
42-53:⚠️ Potential issue | 🟠 MajorPersist the computed behavior knobs on the generated profiles.
active_hours,activity,influence, and the response-delay settings are derived fromAgentMappingConfigbut never attached toOasisAgentProfileor exported as metadata. Right now most of the config surface is a no-op from the simulation's point of view.Also applies to: 126-177, 257-305, 391-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/entity_mapper.py` around lines 42 - 53, The computed behavior knobs (active_hours, activity, influence and the response-delay settings machine_response_delay, operator_response_delay, job_response_delay) are currently derived from AgentMappingConfig but never saved; update the profile construction code that builds OasisAgentProfile (where profiles are created/returned) to persist these values on each OasisAgentProfile instance or include them in exported metadata (e.g., a profile.metadata dict or explicit fields). Specifically, when mapping via AgentMappingConfig, set OasisAgentProfile.active_hours, .activity, .influence and the three response-delay properties (or add them to OasisAgentProfile.metadata) so downstream simulation code can read them; apply the same change in each profile-generation block referenced (the other ranges where profiles are built) to ensure consistency. Ensure types match existing model (ints/bools/lists) and include the AgentMappingConfig source values on the profile object returned.backend/app/services/digital_twin/db_integration.py-418-430 (1)
418-430:⚠️ Potential issue | 🟠 MajorPopulate
Operation.predecessorsfrom the ERP routing sequence.The solver only enforces precedence through
Operation.predecessors, but this adapter drops thesequenceinformation and returns every operation with an empty predecessor list. Jobs loaded from ERP can therefore run all steps in parallel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/db_integration.py` around lines 418 - 430, The adapter currently ignores ERP routing sequence so each Operation has empty predecessors; fix by reading the routing sequence for each row (e.g., row["sequence"] or the actual ERP sequence column), group operations by job (row["job_id"]) while creating Operation instances, then after creating them sort each job's ops by sequence and set Operation.predecessors to the previous step's operation_id for each operation (use the existing operation_id field on Operation instances). Update the loop that builds operations (the for row in rows block) to collect sequence and job_id, and add the predecessor assignment pass before returning the operations list.backend/app/services/digital_twin/disruption_engine.py-389-413 (1)
389-413:⚠️ Potential issue | 🟠 MajorThis simulator cannot emit operator-absence predictions yet.
It fetches machine states instead of operator states, and
_get_operator_ids()always returns[]. The loop never runs, so the operator-absence path is effectively disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/disruption_engine.py` around lines 389 - 413, The operator-absence path is disabled because the code calls self.state_manager.get_all_machine_states() and _get_operator_ids() returns an empty list; fix by retrieving real operator IDs from the state manager and iterating over them: replace the placeholder fetch with a query that returns operator states (e.g., call a method like state_manager.get_all_operator_states() or filter state_manager.get_all_states() for type "operator") and implement _get_operator_ids to return the list of operator IDs extracted from those states so the loop that calls _calculate_absence_probability and _create_absence_prediction actually runs.backend/app/services/digital_twin/disruption_engine.py-240-277 (1)
240-277:⚠️ Potential issue | 🟠 Major
base_failure_ratecurrently does not influence machine breakdown predictions.The calculation uses MTBF, condition factors, and
machine_aggression, but never readsscenario.base_failure_rate. Changing that scenario knob or the preset scenarios therefore does not change breakdown risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/disruption_engine.py` around lines 240 - 277, The scenario.base_failure_rate is never used; incorporate it into the breakdown probability by applying it to the base_prob before combining other modifiers: read scenario.base_failure_rate (default to 1.0 if None), multiply base_prob by that value (e.g., adjusted_base = base_prob * scenario.base_failure_rate) then compute final_prob = min(0.95, adjusted_base * condition_factor * scenario.machine_aggression); reference symbols: MTBF_BY_TYPE, MachineType, base_prob, condition_factor, scenario.simulation_hours, scenario.base_failure_rate, scenario.machine_aggression, final_prob; also validate/base-clamp the base_failure_rate to a sensible range (e.g., >=0) to avoid negative/invalid probabilities.backend/app/services/digital_twin/state_manager.py-658-663 (1)
658-663:⚠️ Potential issue | 🟠 MajorMake snapshots independent from live state.
dict(self._machines)/dict(self._operators)/dict(self._jobs)only copy the dictionaries. TheMachineState,OperatorState, andJobStateinstances inside the snapshot stay shared with the manager, so later mutations rewrite previously returned snapshots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/state_manager.py` around lines 658 - 663, The snapshot currently only shallow-copies the top-level dicts (machines/operators/jobs) so the MachineState/OperatorState/JobState objects remain shared and later mutations change prior snapshots; fix by deep-copying the state objects when building FactorySnapshot (e.g., use copy.deepcopy(self._machines), copy.deepcopy(self._operators), copy.deepcopy(self._jobs>) or call each state's clone/serialize method if available) so FactorySnapshot gets independent instances of MachineState/OperatorState/JobState; add the necessary import (from copy import deepcopy) or use the existing clone/serialization helpers and pass those deep-copied structures into FactorySnapshot.backend/app/services/digital_twin/state_manager.py-751-766 (1)
751-766:⚠️ Potential issue | 🟠 MajorInvoke subscribers after releasing the manager lock.
Every mutating caller reaches
_publish_event()from insidewith self._lock:. One slow callback now blocks all state updates, and a callback that subscribes or unsubscribes can mutate_subscriberswhile this loop is iterating it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/state_manager.py` around lines 751 - 766, _publish_event currently invokes subscriber callbacks while the manager lock (self._lock) is held, which allows slow callbacks to block state updates and permits mutations to self._subscribers during iteration; fix by performing the minimal state mutation under the lock (append to self._event_history and increment self._metrics) and then capture a snapshot of subscribers (e.g., subscribers_snapshot = list(self._subscribers.items())) while still holding the lock, release the lock, and iterate over subscribers_snapshot to call callback(event) inside try/except; this prevents blocking other callers and avoids concurrent mutation during iteration while preserving updates to _event_history and _metrics.backend/app/services/scheduling/solver.py-754-758 (1)
754-758:⚠️ Potential issue | 🟠 MajorCheck
machine.machine_typeagainst the alternate list.The current condition inspects
op.machine_typeinstead of the candidate machine being iterated. Alternate routes are skipped entirely, and if the primary type is accidentally listed as an alternate this branch admits every remaining machine.Minimal fix
for machine in problem.machines: if machine.machine_type == op.machine_type: valid.append(machine) - elif op.machine_type in op.alternative_machine_types: + elif machine.machine_type in op.alternative_machine_types: valid.append(machine)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/solver.py` around lines 754 - 758, The loop that builds valid machines compares op.machine_type to op.alternative_machine_types instead of checking the candidate machine's type; update the elif in the loop over problem.machines so it checks whether machine.machine_type is in op.alternative_machine_types (leave the existing direct equality check for machine.machine_type == op.machine_type and append to valid as before).backend/app/services/scheduling/models.py-455-464 (1)
455-464:⚠️ Potential issue | 🟠 MajorCheck alternate machine availability correctly during validation.
The inner condition compares
op.machine_typeagainstop.alternative_machine_typesinstead of checking whether any alternate exists inmachine_types. An operation with no primary machine but a valid alternate still fails validation.Minimal fix
machine_types = {m.machine_type for m in self.machines} for job in self.jobs: for op in job.operations: if op.machine_type not in machine_types: - if op.machine_type not in op.alternative_machine_types: + if not any( + alt_type in machine_types + for alt_type in op.alternative_machine_types + ): errors.append( f"Job {job.job_id} op {op.operation_id}: " f"no machine for {op.machine_type.value}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/scheduling/models.py` around lines 455 - 464, The validation currently checks if op.machine_type is missing then incorrectly tests op.machine_type against op.alternative_machine_types; update the logic in the block using machine_types, jobs, operations, op.machine_type, and op.alternative_machine_types so that when op.machine_type is not in machine_types you only add an error if none of the alternatives are available — i.e., replace the inner check with a test like "if not any(alt in machine_types for alt in op.alternative_machine_types)" (handling empty/None alternative lists appropriately) before appending to errors.backend/app/services/digital_twin/db_integration.py-196-205 (1)
196-205:⚠️ Potential issue | 🟠 MajorUse a transactional connection for writes.
execute_query()runs throughengine.connect()without explicit transaction control. In SQLAlchemy 2.x, this does not auto-commit INSERT/UPDATE/DELETE statements on connection exit—they roll back instead. This affects multiple write paths:
- Machine sensor metrics (line 643:
INSERT INTO machine_sensors)- Configuration updates (line 774: UPSERT to config table)
- Predictions (line 838:
INSERT INTO predictions)- State change events (line 896:
INSERT INTO state_change_events)Change
engine.connect()toengine.begin()to auto-commit on success or roll back on exception.Fix
- with engine.connect() as conn: + with engine.begin() as conn: result = conn.execute(text(query), params or {}) return [dict(row._mapping) for row in result]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/db_integration.py` around lines 196 - 205, The execute_query function currently uses engine.connect() which does not auto-commit DML in SQLAlchemy 2.x; change the connection context to a transactional one by replacing engine.connect() with engine.begin() in execute_query so that INSERT/UPDATE/DELETE statements in methods that call execute_query (e.g., machine sensor metrics, config upserts, predictions, state_change_events) will be committed on success and rolled back on exception; keep the existing text(query) and params handling and return logic unchanged.backend/app/services/digital_twin/prediction_bridge.py-405-410 (1)
405-410:⚠️ Potential issue | 🟠 MajorUse
copy.deepcopy()instead ofcopy.copy()to avoid mutating the original problem.
copy.copy(problem)only duplicates the top-levelSchedulingProblemobject. The nestedmachines,operators, andjobslists remain shared references, so mutations in_apply_machine_unavailability(),_apply_operator_unavailability(), and related methods modify the original problem even when the bridge decides not to adoptupdated_problem.Minimal fix
- return copy.copy(problem) + return copy.deepcopy(problem)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/prediction_bridge.py` around lines 405 - 410, The _copy_problem function currently uses copy.copy(problem) which only performs a shallow copy and leaves nested lists (machines, operators, jobs) shared; change it to use copy.deepcopy(problem) so nested structures are cloned and later mutations in methods like _apply_machine_unavailability and _apply_operator_unavailability do not change the original SchedulingProblem; ensure the import copy remains and call copy.deepcopy(problem) inside _copy_problem.
🟡 Minor comments (2)
backend/app/services/digital_twin/INTEGRATION_GUIDE.md-331-338 (1)
331-338:⚠️ Potential issue | 🟡 MinorKeep the
jsonexamples copy/pasteable.These fenced blocks are labeled as JSON, but the inline
// ...comments make them invalid JSON. Anyone pasting them into a client will hit a parse error. Move the notes outside the object or switch those fences to a non-JSON language.Also applies to: 634-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/digital_twin/INTEGRATION_GUIDE.md` around lines 331 - 338, The fenced JSON examples (e.g., the object containing "scenario", "reschedule_strategy", and "current_problem") contain inline `// ...` comments which make them invalid copy/pasteable JSON; fix by removing the inline comments from inside the JSON objects (move explanatory notes outside the fenced block) or change the fence language from ```json to ```text or ```js so the examples can include comments; apply the same change for the other affected block around the "current_problem" example (also referenced at lines 634-640).STRATEGIC_PLAN.md-817-834 (1)
817-834:⚠️ Potential issue | 🟡 MinorResolve the remaining placeholders before publishing this plan.
Appendix B and the footer still ship placeholders like
[GitHub link],[Academic paper],[Stakeholder names], and[Date]. That makes the document hard to validate or circulate outside the immediate draft context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@STRATEGIC_PLAN.md` around lines 817 - 834, The References section (heading "### B. References") and the footer fields in STRATEGIC_PLAN.md contain unresolved placeholders like "[GitHub link]", "[Academic paper]", "[Stakeholder names]", and "[Date]"; replace each placeholder with the final, authoritative information (actual URLs for OASIS Framework and MiroFish Original, full citation(s) for the academic paper(s) and Job Shop Scheduling research, the concrete reviewer names for "Reviewers", and a calendar date for "Next Review Date"), or if items are not yet available add an explicit TODO tag with the owner and deadline (e.g., "TODO: provide URL — `@alice` by 2024-04-15") so reviewers can validate the document before publishing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/digital_twin.py`:
- Around line 177-233: The initialize_service handler currently allows
unauthenticated rewiring of DatabaseConfig and digital_twin_service; protect it
by enforcing the control-plane auth checks described in INTEGRATION_GUIDE:
verify the request origin and credentials before proceeding (e.g., validate a
configured admin API key/header or check request.remote_addr against the allowed
IP whitelist), and if the check fails return a 403 and log the denied attempt;
implement this check at the start of initialize_service (before constructing
db_configs or calling digital_twin_service.initialize/load_from_erp) and ensure
the denial uses logger.error and does not mutate digital_twin_service state.
- Around line 553-584: The endpoint is using placeholder data
(create_default_scenario, literal Ellipsis in SchedulingProblem) and ignores
incoming parameters; fix simulate/schedule to parse the incoming request payload
(use the provided scenario, reschedule_strategy, and current_problem), convert
the snapshot from digital_twin_service.state_manager.create_snapshot() into real
machine/operator/job lists (replace the Ellipsis in SchedulingProblem with
proper conversion logic), call
digital_twin_service.prediction_bridge.set_current_problem(problem) and
process_simulation_results(...) as done but then return the full rescheduled
schedule object (the new schedule/makepan and recommendations per
INTEGRATION_GUIDE) rather than only summary fields; update code paths around
create_default_scenario, SchedulingProblem construction, and
prediction_bridge.process_simulation_results to ensure the response includes the
complete rescheduled schedule structure documented in INTEGRATION_GUIDE.md.
- Around line 451-479: The async callback code using callback_url in run_async
should validate the URL against an allowlist, add a timeout to requests.post,
and mark the background thread as a daemon: validate callback_url (the value
from data.get("callback_url")) against a configured allowlist of allowed
hostnames/paths before using it, then call
digital_twin_service.disruption_engine.simulate_scenario(scenario) and send the
POST using requests.post(..., timeout=10) (or 10–30s), catch and log exceptions
with full details via logger.error, and create the thread via thread =
threading.Thread(target=run_async); thread.daemon = True; thread.start() so it
won’t block shutdown.
In `@backend/app/services/digital_twin/__init__.py`:
- Around line 15-58: The package __init__.py is missing re-exports required by
callers; add AgentMappingConfig and map_scheduling_problem_to_agents from
.entity_mapper and create_default_scenario and create_high_stress_scenario from
.disruption_engine to the top-level imports and include those four symbols in
the module's __all__ so backend/app/api/digital_twin.py and
backend/app/services/digital_twin/example_usage.py can import them from
digital_twin; update the import tuples that reference .entity_mapper and
.disruption_engine to include these names and add them to the existing __all__
list alongside the other exported classes and functions.
In `@backend/app/services/digital_twin/db_integration.py`:
- Around line 352-374: The SQL is vulnerable because caller inputs
(status_filter, date_range, etc.) are string-interpolated into the query;
instead build the query using parameterized placeholders and pass values to the
DB driver (e.g., use parameter binding in the function that constructs query for
m.jobs_table), replacing the f-string fragments that reference status_filter and
date_range with safe placeholders and a params list; for status_filter convert
the list into the appropriate number of placeholders (e.g., IN (%s,...)) and
extend params with the values, for date_range add two placeholders for start and
end and append those values, and then execute the final query string (the
variable query) with the params array so m.job_status_column,
m.job_due_date_column, m.job_id_column, etc. remain column identifiers but all
caller-supplied values are bound parameters; apply the same pattern to the other
occurrences mentioned (metric_types, time_range).
In `@backend/app/services/digital_twin/example_usage.py`:
- Around line 331-350: The dynamic import using __import__ inside the
predictions construction should be replaced with a static import of
DisruptionType and the predictions entry updated to reference it directly; add
"from backend.app.services.digital_twin.disruption_engine import DisruptionType"
at the top of the module and change the object literal in the predictions list
(the type(...) call that sets "disruption_type") to use
DisruptionType.MACHINE_BREAKDOWN instead of the __import__ expression so the
symbol resolves reliably at import time.
In `@backend/app/services/scheduling/historical_data.py`:
- Around line 145-148: The relative import in both _get_logger methods is
incorrect: change the import that currently reads from ....utils.logger to use
two dots so it imports from ..utils.logger (i.e., import get_logger from
backend.app.utils.logger); update the _get_logger implementations in the
HistoricalDataLoader and ConstraintCalibrator classes to use "from
..utils.logger import get_logger" so calls to
get_logger("mirofish.scheduling.HistoricalDataLoader") and
get_logger("mirofish.scheduling.ConstraintCalibrator") resolve at runtime.
In `@backend/app/services/scheduling/solver.py`:
- Around line 235-256: The fallback that returns [0] in _get_valid_machines is
incorrect; instead of inventing machine 0, detect the no-compatible-machine case
and raise a clear exception (e.g., ValueError or a SchedulingError) from
_get_valid_machines with context (operation id/name and required machine_type
and alternative_machine_types) so the caller (_build_schedule or higher) can
reject the problem; replace the final return line so it raises the error when
unique_valid is empty rather than returning [0], and keep references to
self.machine_map and self.machine_list in the error message for debugging.
- Around line 663-691: The loop that dispatches operations from sorted_ops must
only schedule an operation when all its predecessors are already in
op_completion_time; change the scheduling loop in the solver (the block using
_sort_operations, op.predecessors, op_completion_time, _get_valid_machines) to
process sorted_ops as a FIFO queue: pop an entry, check that every pred_key =
(job.job_id, pred_id) exists in op_completion_time, and if any are missing
re-queue the operation to the tail; only proceed with the existing
machine-selection logic when all predecessors are present. Also add a simple
cycle-detection/limit (e.g., count iterations or detect no-progress) to avoid
infinite loops and surface an error if dependencies cannot be satisfied.
In `@backend/app/services/scheduling/visualization.py`:
- Around line 62-65: The relative import in ScheduleVisualizer._get_logger is
using four dots ("....utils.logger") which resolves to backend.utils.logger and
will ImportError; update the import to use three dots so it imports
backend.app.utils.logger (i.e., change the module path used when importing
get_logger in the _get_logger method) ensuring get_logger is imported from the
correct package.
---
Major comments:
In `@ARCHITECTURE_OPTIONS.md`:
- Around line 285-323: The abstract Scenario contract is inconsistent with the
JobShopScenario example: load_entities and map_to_agents are typed as List[Any]
but JobShopScenario.load_entities returns a keyed mapping accessed by
map_to_agents (entities['machines'], etc.); update the interface to use a shared
mapping type (e.g., EntitySet or Dict[str, List[Any>]) so the contract matches
the plugin example. Concretely, change the abstract methods in
Scenario—load_entities, map_to_agents, and any related docstrings—to
accept/return the unified mapping type and update signatures/comments to
reference EntitySet (or Dict[str, List[Any>]); ensure JobShopScenario keeps its
implementation and that map_to_agents's parameter type matches the new EntitySet
type.
In `@backend/app/api/digital_twin.py`:
- Around line 230-232: The current except blocks that do "except Exception as e"
(seen in the route handler containing the logger.error(f\"Failed to initialize:
{e}\") and jsonify(... str(e) ...)) leak internal error details to callers and
logs; change these handlers to return a generic error response (e.g.
jsonify({"success": False, "error": "Internal server error"}) with a 500) and
instead log a sanitized, non-sensitive message plus safe context (e.g.
logger.exception or logger.error with a message like "Failed to initialize
digital twin" and include only non-sensitive identifiers), and apply the same
pattern to the other route handlers that currently return str(e) so internal
DB/mapping details are not exposed.
In `@backend/app/services/digital_twin/db_integration.py`:
- Around line 608-621: The loop in get_machine_metrics (iterating over metrics)
overwrites availability/performance/quality for every matching row, causing OEE
to be computed from the oldest row; change the logic to keep the first (newest)
value found per metric type by only assigning result["availability"],
result["performance"], and result["quality"] if they are not already set (or are
None/uninitialized), and stop early once all three metrics are populated (or
break when result has all three keys) so the first encountered (most recent) row
for each metric is preserved before computing result["oee"].
- Around line 418-430: The adapter currently ignores ERP routing sequence so
each Operation has empty predecessors; fix by reading the routing sequence for
each row (e.g., row["sequence"] or the actual ERP sequence column), group
operations by job (row["job_id"]) while creating Operation instances, then after
creating them sort each job's ops by sequence and set Operation.predecessors to
the previous step's operation_id for each operation (use the existing
operation_id field on Operation instances). Update the loop that builds
operations (the for row in rows block) to collect sequence and job_id, and add
the predecessor assignment pass before returning the operations list.
- Around line 196-205: The execute_query function currently uses
engine.connect() which does not auto-commit DML in SQLAlchemy 2.x; change the
connection context to a transactional one by replacing engine.connect() with
engine.begin() in execute_query so that INSERT/UPDATE/DELETE statements in
methods that call execute_query (e.g., machine sensor metrics, config upserts,
predictions, state_change_events) will be committed on success and rolled back
on exception; keep the existing text(query) and params handling and return logic
unchanged.
In `@backend/app/services/digital_twin/disruption_engine.py`:
- Around line 389-413: The operator-absence path is disabled because the code
calls self.state_manager.get_all_machine_states() and _get_operator_ids()
returns an empty list; fix by retrieving real operator IDs from the state
manager and iterating over them: replace the placeholder fetch with a query that
returns operator states (e.g., call a method like
state_manager.get_all_operator_states() or filter state_manager.get_all_states()
for type "operator") and implement _get_operator_ids to return the list of
operator IDs extracted from those states so the loop that calls
_calculate_absence_probability and _create_absence_prediction actually runs.
- Around line 240-277: The scenario.base_failure_rate is never used; incorporate
it into the breakdown probability by applying it to the base_prob before
combining other modifiers: read scenario.base_failure_rate (default to 1.0 if
None), multiply base_prob by that value (e.g., adjusted_base = base_prob *
scenario.base_failure_rate) then compute final_prob = min(0.95, adjusted_base *
condition_factor * scenario.machine_aggression); reference symbols:
MTBF_BY_TYPE, MachineType, base_prob, condition_factor,
scenario.simulation_hours, scenario.base_failure_rate,
scenario.machine_aggression, final_prob; also validate/base-clamp the
base_failure_rate to a sensible range (e.g., >=0) to avoid negative/invalid
probabilities.
In `@backend/app/services/digital_twin/entity_mapper.py`:
- Around line 42-53: The computed behavior knobs (active_hours, activity,
influence and the response-delay settings machine_response_delay,
operator_response_delay, job_response_delay) are currently derived from
AgentMappingConfig but never saved; update the profile construction code that
builds OasisAgentProfile (where profiles are created/returned) to persist these
values on each OasisAgentProfile instance or include them in exported metadata
(e.g., a profile.metadata dict or explicit fields). Specifically, when mapping
via AgentMappingConfig, set OasisAgentProfile.active_hours, .activity,
.influence and the three response-delay properties (or add them to
OasisAgentProfile.metadata) so downstream simulation code can read them; apply
the same change in each profile-generation block referenced (the other ranges
where profiles are built) to ensure consistency. Ensure types match existing
model (ints/bools/lists) and include the AgentMappingConfig source values on the
profile object returned.
In `@backend/app/services/digital_twin/prediction_bridge.py`:
- Around line 434-448: The _apply_operator_unavailability currently makes an
operator permanently unavailable by setting operator.shift_start and
operator.shift_end to 0; instead use the constraint's absence_time and
duration_minutes to create a time-bounded unavailability window. Locate
_apply_operator_unavailability and, when matching operator.operator_id, parse
constraint["absence_time"] and constraint["duration_minutes"], compute the start
and end timestamps (or time slots) for the absence, and either (a) add this
interval to the operator's existing unavailability list/property (e.g.,
operator.unavailabilities.append(...) or operator.add_unavailability(...)) or
(b) adjust shift_start/shift_end only for that interval rather than globally so
future solves respect availability outside the absence window; ensure you do not
overwrite permanent shift fields and that the code uses the
SchedulingProblem/operator APIs to record time-bounded constraints.
- Around line 405-410: The _copy_problem function currently uses
copy.copy(problem) which only performs a shallow copy and leaves nested lists
(machines, operators, jobs) shared; change it to use copy.deepcopy(problem) so
nested structures are cloned and later mutations in methods like
_apply_machine_unavailability and _apply_operator_unavailability do not change
the original SchedulingProblem; ensure the import copy remains and call
copy.deepcopy(problem) inside _copy_problem.
In `@backend/app/services/digital_twin/state_manager.py`:
- Around line 785-798: load_state currently reads JSON but never rebuilds
in-memory state; update load_state to deserialize the loaded JSON into actual
MachineState, OperatorState, JobState (and any Event objects) and repopulate the
manager's internal containers (_machines, _operators, _jobs, _event_history).
Specifically, parse the expected keys from the JSON snapshot, call the
appropriate constructors or classmethod deserializers on
MachineState/OperatorState/JobState to recreate instances, replace/clear and set
self._machines/self._operators/self._jobs (preserving keys/IDs), restore
self._event_history as a list of event objects, validate required fields and
raise/log errors on mismatch, and only return True when reconstruction succeeds
(return False on validation/deserialization failure). Ensure load_state
references the existing serialization format and uses the same field names the
save routine emits.
- Around line 658-663: The snapshot currently only shallow-copies the top-level
dicts (machines/operators/jobs) so the MachineState/OperatorState/JobState
objects remain shared and later mutations change prior snapshots; fix by
deep-copying the state objects when building FactorySnapshot (e.g., use
copy.deepcopy(self._machines), copy.deepcopy(self._operators),
copy.deepcopy(self._jobs>) or call each state's clone/serialize method if
available) so FactorySnapshot gets independent instances of
MachineState/OperatorState/JobState; add the necessary import (from copy import
deepcopy) or use the existing clone/serialization helpers and pass those
deep-copied structures into FactorySnapshot.
- Around line 751-766: _publish_event currently invokes subscriber callbacks
while the manager lock (self._lock) is held, which allows slow callbacks to
block state updates and permits mutations to self._subscribers during iteration;
fix by performing the minimal state mutation under the lock (append to
self._event_history and increment self._metrics) and then capture a snapshot of
subscribers (e.g., subscribers_snapshot = list(self._subscribers.items())) while
still holding the lock, release the lock, and iterate over subscribers_snapshot
to call callback(event) inside try/except; this prevents blocking other callers
and avoids concurrent mutation during iteration while preserving updates to
_event_history and _metrics.
In `@backend/app/services/scheduling/historical_data.py`:
- Around line 448-456: The code is using job.material when querying historical
aggregates keyed by HistoricalJobRecord.job_type, causing misses; update the
loader calls (e.g., get_average_job_duration and the confidence scoring call
around job.historical_similar_job_duration and the block at lines ~472-474) to
pass the real job type (job.job_type or the correct job_type property) instead
of job.material so lookups use HistoricalJobRecord.job_type and return the
intended aggregates.
- Around line 398-405: calibrate_operation() is filtering
op_record["machine_type"] but _load_operations() doesn’t populate machine_type
(it only stores operation_id, machine_id, planned_duration, actual_duration,
setup_time), so similar_ops stays empty; fix by modifying _load_operations() to
include the machine_type (derive it from the machine_id or join the machines
table) when building each op_record so that calibrate_operation() can correctly
compare op_record["machine_type"] to machine_type.value, or alternatively update
calibrate_operation() to map op_record["machine_id"] to the corresponding
machine_type before comparison (use the same machine lookup used elsewhere in
the loader/job_history code).
In `@backend/app/services/scheduling/models.py`:
- Around line 455-464: The validation currently checks if op.machine_type is
missing then incorrectly tests op.machine_type against
op.alternative_machine_types; update the logic in the block using machine_types,
jobs, operations, op.machine_type, and op.alternative_machine_types so that when
op.machine_type is not in machine_types you only add an error if none of the
alternatives are available — i.e., replace the inner check with a test like "if
not any(alt in machine_types for alt in op.alternative_machine_types)" (handling
empty/None alternative lists appropriately) before appending to errors.
In `@backend/app/services/scheduling/solver.py`:
- Around line 435-465: total_tardiness is never linked to the per-job tardiness
vars so the solver can ignore lateness; collect each per-job NewIntVar (created
as tardiness_{job.job_id}) into a list (e.g., job_tardiness_vars) when you
create them, then add a constraint tying total_tardiness to their sum using
self.model.Add(total_tardiness == sum(job_tardiness_vars)); ensure you reference
the same end_vars[(job.job_id, last_op.operation_id)] when creating each
tardiness and keep total_tardiness bounds appropriate (it already is), then
leave the existing objectives.append(total_tardiness *
int(self.config.tardiness_weight * 1000)) as-is so the weight actually affects
the objective.
- Around line 166-198: The solver currently prepares machine metadata
(Machine.status, maintenance_windows) and computes setup times but never encodes
them into the CP-SAT model: update _prepare_data/_prepare_model to translate
maintenance_windows/Machine.status into forbidden intervals (add
NoOverlap2/interval constraints or presence booleans per machine) so tasks
cannot be scheduled during downtime, implement _add_setup_time_constraints to
add precedence/transition constraints (sequence-dependent setup as transition
times between interval variables for operations assigned to the same machine,
using AddNoOverlap/NoOverlap2 with transition matrix or pairwise precedences),
and fully implement _add_resource_constraints to model operator/labor capacity
(resource capacity constraints or cumulative constraints linking operation
intervals to resource demands). Use existing symbols machine_map,
operation_list, _add_setup_time_constraints, _add_resource_constraints, and
horizon to locate the spots to add forbidden-interval creation,
interval-to-machine assignment, transition matrices for setup, and resource
capacity enforcement so unavailable machines and sequence-dependent setups are
respected by the CP-SAT model.
- Around line 817-821: The current return logic compares
optimized_schedule.makespan against heuristic_schedule.makespan even when the
CP-SAT solve was infeasible/unknown (JobShopSolver._build_schedule() returns
makespan=0 for those cases), causing an unusable schedule to be chosen; modify
the logic to first detect whether the CP-SAT result is feasible/optimal (or that
optimized_schedule is valid) before comparing makespans — e.g., check the solver
result status or a validity flag produced by _build_schedule() for
optimized_schedule and only fall back to comparing makespan if
optimized_schedule is feasible/valid, otherwise return heuristic_schedule.
Ensure you reference optimized_schedule, heuristic_schedule and
JobShopSolver._build_schedule() when implementing the check.
- Around line 754-758: The loop that builds valid machines compares
op.machine_type to op.alternative_machine_types instead of checking the
candidate machine's type; update the elif in the loop over problem.machines so
it checks whether machine.machine_type is in op.alternative_machine_types (leave
the existing direct equality check for machine.machine_type == op.machine_type
and append to valid as before).
In `@backend/app/services/scheduling/visualization.py`:
- Around line 484-488: The current total_wait calculation uses (e.start_time -
e.end_time) which is non-positive and always zeroes the queue metric; change the
logic in the block that computes total_wait for machine_entries to compute gaps
between consecutive operations instead: sort or iterate machine_entries in
chronological order and for each entry compute wait = max(0, (current.start_time
- previous.end_time).total_seconds() / 60) (or use a real ready_time if you have
one on the entry), sum those waits into total_wait (or total_wait_minutes) so
the queue-time reflects actual inter-operation queuing rather than e.start_time
- e.end_time.
- Around line 187-205: The generate_utilization_heatmap function currently
ignores the window_hours parameter and always spans from the earliest entry to
the latest; update it to honor window_hours by bounding the time range: compute
raw_start = min(e.start_time ...) and raw_end = max(e.end_time ...), then set
start_time = max(raw_start, raw_end - timedelta(hours=window_hours)) (and
validate window_hours > 0), keep end_time = raw_end, and then build the hourly
buckets using current = start_time with the existing loop over hours/current;
ensure any downstream logic that uses hours, start_time or end_time uses the new
bounded range.
---
Minor comments:
In `@backend/app/services/digital_twin/INTEGRATION_GUIDE.md`:
- Around line 331-338: The fenced JSON examples (e.g., the object containing
"scenario", "reschedule_strategy", and "current_problem") contain inline `//
...` comments which make them invalid copy/pasteable JSON; fix by removing the
inline comments from inside the JSON objects (move explanatory notes outside the
fenced block) or change the fence language from ```json to ```text or ```js so
the examples can include comments; apply the same change for the other affected
block around the "current_problem" example (also referenced at lines 634-640).
In `@STRATEGIC_PLAN.md`:
- Around line 817-834: The References section (heading "### B. References") and
the footer fields in STRATEGIC_PLAN.md contain unresolved placeholders like
"[GitHub link]", "[Academic paper]", "[Stakeholder names]", and "[Date]";
replace each placeholder with the final, authoritative information (actual URLs
for OASIS Framework and MiroFish Original, full citation(s) for the academic
paper(s) and Job Shop Scheduling research, the concrete reviewer names for
"Reviewers", and a calendar date for "Next Review Date"), or if items are not
yet available add an explicit TODO tag with the owner and deadline (e.g., "TODO:
provide URL — `@alice` by 2024-04-15") so reviewers can validate the document
before publishing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 672cace2-2705-45d1-8411-8b7125d54286
📒 Files selected for processing (17)
ARCHITECTURE_OPTIONS.mdSTRATEGIC_PLAN.mdbackend/app/api/digital_twin.pybackend/app/services/digital_twin/INTEGRATION_GUIDE.mdbackend/app/services/digital_twin/README.mdbackend/app/services/digital_twin/__init__.pybackend/app/services/digital_twin/db_integration.pybackend/app/services/digital_twin/disruption_engine.pybackend/app/services/digital_twin/entity_mapper.pybackend/app/services/digital_twin/example_usage.pybackend/app/services/digital_twin/prediction_bridge.pybackend/app/services/digital_twin/state_manager.pybackend/app/services/scheduling/__init__.pybackend/app/services/scheduling/historical_data.pybackend/app/services/scheduling/models.pybackend/app/services/scheduling/solver.pybackend/app/services/scheduling/visualization.py
✅ Files skipped from review due to trivial changes (1)
- backend/app/services/digital_twin/README.md
Comment on lines
+177
to
+233
| @digital_twin_bp.route("/initialize", methods=["POST"]) | ||
| def initialize_service(): | ||
| """ | ||
| Initialize Digital Twin with database configurations. | ||
|
|
||
| Request Body: | ||
| { | ||
| "databases": { | ||
| "erp": {"host": "...", "port": 5432, "database": "...", "username": "...", "password": "..."}, | ||
| "sensor": {...}, | ||
| "dt": {...} | ||
| }, | ||
| "table_mapping": { | ||
| "machines_table": "equipment", | ||
| "machine_id_column": "asset_id", | ||
| ... | ||
| } | ||
| } | ||
|
|
||
| Returns: | ||
| {"success": true, "data": {"initialized": true}} | ||
| """ | ||
| try: | ||
| data = request.get_json() or {} | ||
|
|
||
| # Parse database configs | ||
| db_configs = {} | ||
| for db_name, config in data.get("databases", {}).items(): | ||
| db_configs[f"{db_name}_config"] = DatabaseConfig(name=db_name, **config) | ||
|
|
||
| # Parse table mapping | ||
| mapping = TableMapping(**data.get("table_mapping", {})) | ||
|
|
||
| # Initialize service | ||
| digital_twin_service.initialize(db_configs, mapping) | ||
| digital_twin_service.load_from_erp(mapping) | ||
|
|
||
| return jsonify( | ||
| { | ||
| "success": True, | ||
| "data": { | ||
| "initialized": True, | ||
| "machines_tracked": len( | ||
| digital_twin_service.state_manager.get_all_machine_states() | ||
| ), | ||
| "operators_tracked": len( | ||
| digital_twin_service.state_manager._operators | ||
| ), | ||
| "jobs_tracked": len(digital_twin_service.state_manager._jobs), | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to initialize: {e}") | ||
| return jsonify({"success": False, "error": str(e)}), 500 | ||
|
|
There was a problem hiding this comment.
Protect /initialize as a privileged control-plane route.
This handler accepts database credentials and rewires the singleton service from request data, but there is no auth or source-IP check anywhere in the blueprint even though backend/app/services/digital_twin/INTEGRATION_GUIDE.md (Lines 48-57) currently describes IP-based access control. Any reachable caller can change runtime state and point the server at arbitrary DB hosts.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 230-230: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/digital_twin.py` around lines 177 - 233, The
initialize_service handler currently allows unauthenticated rewiring of
DatabaseConfig and digital_twin_service; protect it by enforcing the
control-plane auth checks described in INTEGRATION_GUIDE: verify the request
origin and credentials before proceeding (e.g., validate a configured admin API
key/header or check request.remote_addr against the allowed IP whitelist), and
if the check fails return a 403 and log the denied attempt; implement this check
at the start of initialize_service (before constructing db_configs or calling
digital_twin_service.initialize/load_from_erp) and ensure the denial uses
logger.error and does not mutate digital_twin_service state.
Comment on lines
+451
to
+479
| # Check for async callback | ||
| callback_url = data.get("callback_url") | ||
|
|
||
| if callback_url: | ||
| # Run async | ||
| def run_async(): | ||
| try: | ||
| predictions = ( | ||
| digital_twin_service.disruption_engine.simulate_scenario( | ||
| scenario | ||
| ) | ||
| ) | ||
|
|
||
| # Send callback | ||
| import requests | ||
|
|
||
| requests.post( | ||
| callback_url, | ||
| json={ | ||
| "simulation_id": simulation_id, | ||
| "status": "completed", | ||
| "predictions": [p.to_dict() for p in predictions], | ||
| }, | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Async simulation failed: {e}") | ||
|
|
||
| thread = threading.Thread(target=run_async) | ||
| thread.start() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
from pathlib import Path
tree = ast.parse(Path("backend/app/api/digital_twin.py").read_text())
for node in ast.walk(tree):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute):
if getattr(node.func.value, "id", None) == "requests" and node.func.attr == "post":
keywords = [kw.arg for kw in node.keywords if kw.arg is not None]
print("requests.post keywords:", sorted(keywords))
PY
rg -n "callback_url|requests\.post\(" backend/app/api/digital_twin.py -C2Repository: lilubot/MiroFish-Offline
Length of output: 674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any validation of callback_url
rg -n "callback_url" backend/app/api/digital_twin.py -B5 -A10
# Check if there's a global timeout configuration for requests
rg -n "timeout|requests\." backend/app/api/ -t py | head -30
# Look for any URL validation functions
rg -n "validate|allowlist|whitelist|regex|http" backend/app/api/digital_twin.py -i | head -20Repository: lilubot/MiroFish-Offline
Length of output: 3463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check imports and top of file
head -50 backend/app/api/digital_twin.py
# Check for requests Session or global configuration
rg -n "requests\.|Session|timeout" backend/app/api/digital_twin.py -B2 -A2
# Check if thread.daemon is set
rg -n "daemon|Thread\(" backend/app/api/digital_twin.py -B2 -A2Repository: lilubot/MiroFish-Offline
Length of output: 1926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's any global requests configuration or middleware
rg -n "requests" backend/app/api/digital_twin.py | head -20
# Check the full context around the thread creation
sed -n '475,485p' backend/app/api/digital_twin.pyRepository: lilubot/MiroFish-Offline
Length of output: 505
Set a strict timeout and validate the callback URL against an allowlist before making the POST request.
The code posts to a user-supplied callback_url without validation, creating an SSRF primitive. Additionally, requests.post() is called without a timeout parameter. If the callback endpoint is slow or unresponsive, the background thread will hang indefinitely and consume resources. The thread is also not marked as a daemon, so it may persist beyond the application lifecycle.
Implement:
- Allowlist validation for
callback_url(only permit specific domains/paths) timeout=parameter (recommend 10–30 seconds) on therequests.post()call- Optionally set
thread.daemon = Trueto prevent thread stranding
🧰 Tools
🪛 Ruff (0.15.7)
[error] 467-467: Probable use of requests call without timeout
(S113)
[warning] 475-475: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/digital_twin.py` around lines 451 - 479, The async callback
code using callback_url in run_async should validate the URL against an
allowlist, add a timeout to requests.post, and mark the background thread as a
daemon: validate callback_url (the value from data.get("callback_url")) against
a configured allowlist of allowed hostnames/paths before using it, then call
digital_twin_service.disruption_engine.simulate_scenario(scenario) and send the
POST using requests.post(..., timeout=10) (or 10–30s), catch and log exceptions
with full details via logger.error, and create the thread via thread =
threading.Thread(target=run_async); thread.daemon = True; thread.start() so it
won’t block shutdown.
Comment on lines
+553
to
+584
| # Step 1: Run simulation | ||
| scenario = create_default_scenario() | ||
| predictions = digital_twin_service.disruption_engine.simulate_scenario(scenario) | ||
|
|
||
| # Step 2: Build scheduling problem from current state | ||
| snapshot = digital_twin_service.state_manager.create_snapshot() | ||
|
|
||
| # Convert to SchedulingProblem | ||
| problem = SchedulingProblem( | ||
| problem_id=f"twin_{datetime.now().strftime('%Y%m%d_%H%M%S')}", | ||
| name="Digital Twin Optimized", | ||
| machines=[...], # Convert from snapshot | ||
| operators=[...], | ||
| jobs=[...], | ||
| ) | ||
|
|
||
| # Step 3: Process through prediction bridge | ||
| digital_twin_service.prediction_bridge.set_current_problem(problem) | ||
| results = digital_twin_service.prediction_bridge.process_simulation_results( | ||
| predictions, auto_reschedule=True | ||
| ) | ||
|
|
||
| return jsonify( | ||
| { | ||
| "success": True, | ||
| "data": { | ||
| "predictions": [p.to_dict() for p in predictions], | ||
| "reschedule_triggered": results.get("reschedule_triggered"), | ||
| "new_makespan": results.get("new_schedule_makespan"), | ||
| "recommendations": results.get("recommendations", []), | ||
| }, | ||
| } |
There was a problem hiding this comment.
/simulate/schedule is still wired to placeholder data.
It ignores the incoming scenario, reschedule_strategy, and current_problem, constructs SchedulingProblem with literal Ellipsis entries, and only returns summary fields instead of the rescheduled schedule object documented in backend/app/services/digital_twin/INTEGRATION_GUIDE.md (Lines 341-377). In this state, consumers cannot get a valid or actionable schedule from the endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/digital_twin.py` around lines 553 - 584, The endpoint is
using placeholder data (create_default_scenario, literal Ellipsis in
SchedulingProblem) and ignores incoming parameters; fix simulate/schedule to
parse the incoming request payload (use the provided scenario,
reschedule_strategy, and current_problem), convert the snapshot from
digital_twin_service.state_manager.create_snapshot() into real
machine/operator/job lists (replace the Ellipsis in SchedulingProblem with
proper conversion logic), call
digital_twin_service.prediction_bridge.set_current_problem(problem) and
process_simulation_results(...) as done but then return the full rescheduled
schedule object (the new schedule/makepan and recommendations per
INTEGRATION_GUIDE) rather than only summary fields; update code paths around
create_default_scenario, SchedulingProblem construction, and
prediction_bridge.process_simulation_results to ensure the response includes the
complete rescheduled schedule structure documented in INTEGRATION_GUIDE.md.
Comment on lines
+15
to
+58
| from .entity_mapper import ( | ||
| SchedulingEntityMapper, | ||
| MachineAgentMapper, | ||
| OperatorAgentMapper, | ||
| JobAgentMapper, | ||
| create_mapper, | ||
| ) | ||
|
|
||
| from .state_manager import ( | ||
| FactoryStateManager, | ||
| MachineState, | ||
| OperatorState, | ||
| JobState, | ||
| FactorySnapshot, | ||
| ) | ||
|
|
||
| from .disruption_engine import ( | ||
| DisruptionEngine, | ||
| MachineFailureSimulator, | ||
| OperatorAvailabilitySimulator, | ||
| RushOrderSimulator, | ||
| DisruptionPrediction, | ||
| ) | ||
|
|
||
| from .prediction_bridge import ( | ||
| PredictionBridge, | ||
| SimulationResultProcessor, | ||
| ConstraintUpdater, | ||
| ReschedulingTrigger, | ||
| ) | ||
|
|
||
| from .db_integration import ( | ||
| # Database Configuration | ||
| DatabaseConfig, | ||
| TableMapping, | ||
| # Connection Management | ||
| DatabaseConnectionManager, | ||
| # Adapters | ||
| ERPAdapter, | ||
| SensorDataAdapter, | ||
| DigitalTwinRepository, | ||
| # Factory Functions | ||
| create_db_manager, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
from pathlib import Path
root = Path("backend/app/services/digital_twin")
def exported_names(path: Path) -> set[str]:
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.Assign) and any(
isinstance(target, ast.Name) and target.id == "__all__"
for target in node.targets
):
return {
elt.value
for elt in node.value.elts
if isinstance(elt, ast.Constant) and isinstance(elt.value, str)
}
return set()
def defined_names(path: Path) -> set[str]:
tree = ast.parse(path.read_text())
return {
node.name
for node in ast.walk(tree)
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef))
}
exports = exported_names(root / "__init__.py")
checks = {
"entity_mapper.py": ["AgentMappingConfig", "map_scheduling_problem_to_agents"],
"disruption_engine.py": ["create_default_scenario", "create_high_stress_scenario"],
}
for filename, names in checks.items():
defined = defined_names(root / filename)
print(filename)
for name in names:
print(f" {name}: defined={name in defined}, exported={name in exports}")
PY
rg -n "services\.digital_twin import" backend/app/api/digital_twin.py backend/app/services/digital_twin/example_usage.py -C2Repository: lilubot/MiroFish-Offline
Length of output: 1033
Add missing re-exports to complete the public API surface.
backend/app/api/digital_twin.py (line 27) and backend/app/services/digital_twin/example_usage.py (line 31) import create_default_scenario, create_high_stress_scenario, AgentMappingConfig, and map_scheduling_problem_to_agents from the digital_twin package, but these names are not re-exported in __init__.py. Add them to both the import statements and the __all__ list:
AgentMappingConfigandmap_scheduling_problem_to_agentsfrom.entity_mappercreate_default_scenarioandcreate_high_stress_scenariofrom.disruption_engine
Suggested export fix
from .entity_mapper import (
SchedulingEntityMapper,
MachineAgentMapper,
OperatorAgentMapper,
JobAgentMapper,
+ AgentMappingConfig,
+ map_scheduling_problem_to_agents,
create_mapper,
)
from .disruption_engine import (
DisruptionEngine,
MachineFailureSimulator,
OperatorAvailabilitySimulator,
RushOrderSimulator,
DisruptionPrediction,
+ create_default_scenario,
+ create_high_stress_scenario,
)📝 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.
Suggested change
| from .entity_mapper import ( | |
| SchedulingEntityMapper, | |
| MachineAgentMapper, | |
| OperatorAgentMapper, | |
| JobAgentMapper, | |
| create_mapper, | |
| ) | |
| from .state_manager import ( | |
| FactoryStateManager, | |
| MachineState, | |
| OperatorState, | |
| JobState, | |
| FactorySnapshot, | |
| ) | |
| from .disruption_engine import ( | |
| DisruptionEngine, | |
| MachineFailureSimulator, | |
| OperatorAvailabilitySimulator, | |
| RushOrderSimulator, | |
| DisruptionPrediction, | |
| ) | |
| from .prediction_bridge import ( | |
| PredictionBridge, | |
| SimulationResultProcessor, | |
| ConstraintUpdater, | |
| ReschedulingTrigger, | |
| ) | |
| from .db_integration import ( | |
| # Database Configuration | |
| DatabaseConfig, | |
| TableMapping, | |
| # Connection Management | |
| DatabaseConnectionManager, | |
| # Adapters | |
| ERPAdapter, | |
| SensorDataAdapter, | |
| DigitalTwinRepository, | |
| # Factory Functions | |
| create_db_manager, | |
| ) | |
| from .entity_mapper import ( | |
| SchedulingEntityMapper, | |
| MachineAgentMapper, | |
| OperatorAgentMapper, | |
| JobAgentMapper, | |
| AgentMappingConfig, | |
| map_scheduling_problem_to_agents, | |
| create_mapper, | |
| ) | |
| from .state_manager import ( | |
| FactoryStateManager, | |
| MachineState, | |
| OperatorState, | |
| JobState, | |
| FactorySnapshot, | |
| ) | |
| from .disruption_engine import ( | |
| DisruptionEngine, | |
| MachineFailureSimulator, | |
| OperatorAvailabilitySimulator, | |
| RushOrderSimulator, | |
| DisruptionPrediction, | |
| create_default_scenario, | |
| create_high_stress_scenario, | |
| ) | |
| from .prediction_bridge import ( | |
| PredictionBridge, | |
| SimulationResultProcessor, | |
| ConstraintUpdater, | |
| ReschedulingTrigger, | |
| ) | |
| from .db_integration import ( | |
| # Database Configuration | |
| DatabaseConfig, | |
| TableMapping, | |
| # Connection Management | |
| DatabaseConnectionManager, | |
| # Adapters | |
| ERPAdapter, | |
| SensorDataAdapter, | |
| DigitalTwinRepository, | |
| # Factory Functions | |
| create_db_manager, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/digital_twin/__init__.py` around lines 15 - 58, The
package __init__.py is missing re-exports required by callers; add
AgentMappingConfig and map_scheduling_problem_to_agents from .entity_mapper and
create_default_scenario and create_high_stress_scenario from .disruption_engine
to the top-level imports and include those four symbols in the module's __all__
so backend/app/api/digital_twin.py and
backend/app/services/digital_twin/example_usage.py can import them from
digital_twin; update the import tuples that reference .entity_mapper and
.disruption_engine to include these names and add them to the existing __all__
list alongside the other exported classes and functions.
Comment on lines
+352
to
+374
| status_clause = "" | ||
| if status_filter: | ||
| statuses = ", ".join([f"'{s}'" for s in status_filter]) | ||
| status_clause = f"AND {m.job_status_column} IN ({statuses})" | ||
|
|
||
| date_clause = "" | ||
| if date_range: | ||
| start, end = date_range | ||
| date_clause = f"AND {m.job_due_date_column} BETWEEN '{start}' AND '{end}'" | ||
|
|
||
| query = f""" | ||
| SELECT | ||
| {m.job_id_column} as job_id, | ||
| {m.job_name_column} as name, | ||
| {m.job_priority_column} as priority, | ||
| {m.job_status_column} as status, | ||
| {m.job_due_date_column} as due_date | ||
| FROM {m.jobs_table} | ||
| WHERE 1=1 | ||
| {status_clause} | ||
| {date_clause} | ||
| ORDER BY {m.job_due_date_column} | ||
| """ |
There was a problem hiding this comment.
Parameterize the caller-supplied filters instead of interpolating them into SQL.
status_filter, date_range, metric_types, and time_range are spliced directly into the query text. That opens an injection path and also skips the database driver's normal quoting and type handling for those values.
Also applies to: 569-592
🧰 Tools
🪛 Ruff (0.15.7)
[error] 362-374: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/digital_twin/db_integration.py` around lines 352 - 374,
The SQL is vulnerable because caller inputs (status_filter, date_range, etc.)
are string-interpolated into the query; instead build the query using
parameterized placeholders and pass values to the DB driver (e.g., use parameter
binding in the function that constructs query for m.jobs_table), replacing the
f-string fragments that reference status_filter and date_range with safe
placeholders and a params list; for status_filter convert the list into the
appropriate number of placeholders (e.g., IN (%s,...)) and extend params with
the values, for date_range add two placeholders for start and end and append
those values, and then execute the final query string (the variable query) with
the params array so m.job_status_column, m.job_due_date_column, m.job_id_column,
etc. remain column identifiers but all caller-supplied values are bound
parameters; apply the same pattern to the other occurrences mentioned
(metric_types, time_range).
Comment on lines
+331
to
+350
| predictions = [ | ||
| type( | ||
| "obj", | ||
| (object,), | ||
| { | ||
| "disruption_type": __import__( | ||
| "disruption_engine", fromlist=["DisruptionType"] | ||
| ).DisruptionType.MACHINE_BREAKDOWN, | ||
| "entity_id": "M1", | ||
| "entity_type": "machine", | ||
| "probability": 0.75, | ||
| "predicted_time": datetime.now() + timedelta(hours=4), | ||
| "confidence": 0.8, | ||
| "affected_jobs": ["J1"], | ||
| "estimated_delay_minutes": 120, | ||
| "estimated_cost_impact": 500.0, | ||
| "recommended_action": "Prepare backup machine", | ||
| "alternative_resources": ["M2"], | ||
| }, | ||
| )() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
from pathlib import Path
tree = ast.parse(Path("backend/app/services/digital_twin/example_usage.py").read_text())
for node in ast.walk(tree):
if (
isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id == "__import__"
and node.args
and isinstance(node.args[0], ast.Constant)
):
print("Found __import__ target:", node.args[0].value)
PY
rg -n "__import__|DisruptionType|process_simulation_results" backend/app/services/digital_twin/example_usage.py -C2Repository: lilubot/MiroFish-Offline
Length of output: 802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the disruption_engine module
fd -type f "disruption_engine" backend/
# Check if it exists and its structure
if [ -f "backend/app/services/digital_twin/disruption_engine.py" ]; then
echo "=== disruption_engine.py exists ==="
head -50 "backend/app/services/digital_twin/disruption_engine.py" | cat -n
fi
# Check the __init__.py to see what's exported
if [ -f "backend/app/services/digital_twin/__init__.py" ]; then
echo "=== __init__.py content ==="
cat -n "backend/app/services/digital_twin/__init__.py"
fi
# Search for other imports of DisruptionType
echo "=== Other uses of DisruptionType ==="
rg -n "DisruptionType" backend/ --type py -B2 -A2Repository: lilubot/MiroFish-Offline
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the disruption_engine module
fd "disruption_engine" backend/
# Check if it exists and its structure
if [ -f "backend/app/services/digital_twin/disruption_engine.py" ]; then
echo "=== disruption_engine.py exists ==="
head -50 "backend/app/services/digital_twin/disruption_engine.py" | cat -n
fi
# Check the __init__.py to see what's exported
if [ -f "backend/app/services/digital_twin/__init__.py" ]; then
echo "=== __init__.py content ==="
cat -n "backend/app/services/digital_twin/__init__.py"
fi
# Search for other imports of DisruptionType
echo "=== Other uses of DisruptionType ==="
rg -n "DisruptionType" backend/ -A2 -B2Repository: lilubot/MiroFish-Offline
Length of output: 12196
Use a direct import for DisruptionType instead of dynamic __import__.
The bare __import__("disruption_engine", ...) will fail or load the wrong module, since it searches for a top-level module instead of the package-qualified one. The correct pattern (shown elsewhere in prediction_bridge.py) is to import directly:
from backend.app.services.digital_twin.disruption_engine import DisruptionTypeReplace the dynamic import at line 336-338 with a static import at the top of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/digital_twin/example_usage.py` around lines 331 - 350,
The dynamic import using __import__ inside the predictions construction should
be replaced with a static import of DisruptionType and the predictions entry
updated to reference it directly; add "from
backend.app.services.digital_twin.disruption_engine import DisruptionType" at
the top of the module and change the object literal in the predictions list (the
type(...) call that sets "disruption_type") to use
DisruptionType.MACHINE_BREAKDOWN instead of the __import__ expression so the
symbol resolves reliably at import time.
Comment on lines
+145
to
+148
| def _get_logger(self): | ||
| from ....utils.logger import get_logger | ||
|
|
||
| return get_logger("mirofish.scheduling.HistoricalDataLoader") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from importlib.util import resolve_name
print("current:", resolve_name("....utils.logger", "backend.app.services.scheduling"))
print("expected:", resolve_name("..utils.logger", "backend.app.services.scheduling"))
PYRepository: lilubot/MiroFish-Offline
Length of output: 141
🏁 Script executed:
# Search for logger module locations
fd -t f "logger.py" --max-results 20
# Also check the structure around the imports
git ls-files | grep -E "(utils|logger)" | head -20Repository: lilubot/MiroFish-Offline
Length of output: 311
🏁 Script executed:
# Check if the resolved path exists
test -f backend/utils/logger.py && echo "backend/utils/logger.py EXISTS" || echo "backend/utils/logger.py NOT FOUND"
test -f backend/app/utils/logger.py && echo "backend/app/utils/logger.py EXISTS" || echo "backend/app/utils/logger.py NOT FOUND"
test -f backend/app/services/utils/logger.py && echo "backend/app/services/utils/logger.py EXISTS" || echo "backend/app/services/utils/logger.py NOT FOUND"Repository: lilubot/MiroFish-Offline
Length of output: 183
🏁 Script executed:
# Check lines 350-353 in the file for the second occurrence
sed -n '345,360p' backend/app/services/scheduling/historical_data.pyRepository: lilubot/MiroFish-Offline
Length of output: 529
Fix the relative import for get_logger in both _get_logger methods.
The import from ....utils.logger resolves to backend.utils.logger, which does not exist. The logger module is at backend.app.utils.logger, requiring only two dots: from ..utils.logger.
This affects lines 145-148 (HistoricalDataLoader) and lines 350-353 (ConstraintCalibrator). Either method will fail at runtime on first call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/scheduling/historical_data.py` around lines 145 - 148,
The relative import in both _get_logger methods is incorrect: change the import
that currently reads from ....utils.logger to use two dots so it imports from
..utils.logger (i.e., import get_logger from backend.app.utils.logger); update
the _get_logger implementations in the HistoricalDataLoader and
ConstraintCalibrator classes to use "from ..utils.logger import get_logger" so
calls to get_logger("mirofish.scheduling.HistoricalDataLoader") and
get_logger("mirofish.scheduling.ConstraintCalibrator") resolve at runtime.
Comment on lines
+235
to
+256
| def _get_valid_machines(self, operation: Operation) -> List[int]: | ||
| """Get list of machine indices that can perform this operation""" | ||
| valid = [] | ||
|
|
||
| # Primary machine type | ||
| if operation.machine_type in self.machine_map: | ||
| valid.extend(self.machine_map[operation.machine_type]) | ||
|
|
||
| # Alternative machine types | ||
| for alt_type in operation.alternative_machine_types: | ||
| if alt_type in self.machine_map: | ||
| valid.extend(self.machine_map[alt_type]) | ||
|
|
||
| # Remove duplicates while preserving order | ||
| seen = set() | ||
| unique_valid = [] | ||
| for m in valid: | ||
| if m not in seen: | ||
| seen.add(m) | ||
| unique_valid.append(m) | ||
|
|
||
| return unique_valid if unique_valid else [0] # Fallback |
There was a problem hiding this comment.
Do not invent machine 0 when no compatible resource exists.
_get_valid_machines() falls back to [0]. That either schedules the operation onto an incapable machine or blows up in _build_schedule() when self.machine_list is empty. Reject the problem here instead of manufacturing a resource.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/scheduling/solver.py` around lines 235 - 256, The
fallback that returns [0] in _get_valid_machines is incorrect; instead of
inventing machine 0, detect the no-compatible-machine case and raise a clear
exception (e.g., ValueError or a SchedulingError) from _get_valid_machines with
context (operation id/name and required machine_type and
alternative_machine_types) so the caller (_build_schedule or higher) can reject
the problem; replace the final return line so it raises the error when
unique_valid is empty rather than returning [0], and keep references to
self.machine_map and self.machine_list in the error message for debugging.
Comment on lines
+663
to
+691
| # Sort by dispatch rule | ||
| sorted_ops = self._sort_operations(all_operations) | ||
|
|
||
| # Schedule each operation | ||
| for job, op in sorted_ops: | ||
| # Find earliest start time (after predecessors) | ||
| earliest_start = problem.start_time | ||
|
|
||
| for pred_id in op.predecessors: | ||
| pred_key = (job.job_id, pred_id) | ||
| if pred_key in op_completion_time: | ||
| earliest_start = max(earliest_start, op_completion_time[pred_key]) | ||
|
|
||
| # Find best machine (earliest available) | ||
| valid_machines = self._get_valid_machines(problem, op) | ||
| best_machine = None | ||
| best_start = None | ||
|
|
||
| for machine in valid_machines: | ||
| machine_start = max( | ||
| earliest_start, machine_available_time[machine.machine_id] | ||
| ) | ||
|
|
||
| if best_start is None or machine_start < best_start: | ||
| best_start = machine_start | ||
| best_machine = machine | ||
|
|
||
| if best_machine is None: | ||
| continue # No valid machine |
There was a problem hiding this comment.
Only dispatch operations whose predecessors are already scheduled.
The global sort can place a downstream operation before its predecessor. When that happens, the missing pred_key is treated as “no constraint” and the successor gets scheduled from problem.start_time, which violates the job route.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/scheduling/solver.py` around lines 663 - 691, The loop
that dispatches operations from sorted_ops must only schedule an operation when
all its predecessors are already in op_completion_time; change the scheduling
loop in the solver (the block using _sort_operations, op.predecessors,
op_completion_time, _get_valid_machines) to process sorted_ops as a FIFO queue:
pop an entry, check that every pred_key = (job.job_id, pred_id) exists in
op_completion_time, and if any are missing re-queue the operation to the tail;
only proceed with the existing machine-selection logic when all predecessors are
present. Also add a simple cycle-detection/limit (e.g., count iterations or
detect no-progress) to avoid infinite loops and surface an error if dependencies
cannot be satisfied.
Comment on lines
+62
to
+65
| def _get_logger(self): | ||
| from ....utils.logger import get_logger | ||
|
|
||
| return get_logger("mirofish.scheduling.ScheduleVisualizer") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import importlib.util
package = "backend.app.services.scheduling"
print("....utils.logger ->", importlib.util.resolve_name("....utils.logger", package))
print("...utils.logger ->", importlib.util.resolve_name("...utils.logger", package))
PY
rg -n "from \.{3,4}utils\.logger import get_logger" backend/app/services/scheduling/visualization.py -C1Repository: lilubot/MiroFish-Offline
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Check which logger module exists
echo "=== Checking for logger modules ==="
find backend -type f -name "logger.py" 2>/dev/null | sort
echo ""
echo "=== Directory structure of backend ==="
ls -la backend/ 2>/dev/null | head -20
echo ""
echo "=== Check backend/utils/ ==="
ls -la backend/utils/ 2>/dev/null
echo ""
echo "=== Check backend/app/utils/ ==="
ls -la backend/app/utils/ 2>/dev/nullRepository: lilubot/MiroFish-Offline
Length of output: 1134
Fix the relative import depth for get_logger.
From backend.app.services.scheduling, ....utils.logger resolves to backend.utils.logger, which does not exist. The module is located at backend.app.utils.logger, which requires 3 dots instead of 4. This will raise ImportError when ScheduleVisualizer._get_logger() is called.
Suggested fix
def _get_logger(self):
- from ....utils.logger import get_logger
+ from ...utils.logger import get_logger📝 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.
Suggested change
| def _get_logger(self): | |
| from ....utils.logger import get_logger | |
| return get_logger("mirofish.scheduling.ScheduleVisualizer") | |
| def _get_logger(self): | |
| from ...utils.logger import get_logger | |
| return get_logger("mirofish.scheduling.ScheduleVisualizer") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/scheduling/visualization.py` around lines 62 - 65, The
relative import in ScheduleVisualizer._get_logger is using four dots
("....utils.logger") which resolves to backend.utils.logger and will
ImportError; update the import to use three dots so it imports
backend.app.utils.logger (i.e., change the module path used when importing
get_logger in the _get_logger method) ensuring get_logger is imported from the
correct package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds engineering workflow simulation report tooling to MiroFish-Offline, enabling analysis of Quote → Design → Engineering processes for manufacturing applications.
Changes
New Files (11 files, +5458 lines)
Report Models (
report_models.py):EngineeringReport,EngineeringSection- Structured report data modelsQuoteAccuracyResult- Compare quoted vs actual performanceBottleneckAnalysis- Identify process bottlenecksCollaborationAnalysis- Team collaboration effectivenessDesignQualityResult- Design revision and manufacturability metricsRiskPrediction- Schedule/budget/quality risk assessmentTeamInterviewResult- Structured agent interview responsesScenarioComparisonResult- Multi-scenario outcome comparisonAnalysis Modules (
analysis/):quote_analysis.py- Quote estimation accuracy, margin analysisbottleneck_analysis.py- Workstation utilization, wait times, critical pathcollaboration_analysis.py- Consultation frequency, review effectivenessdesign_quality.py- Revision counts, manufacturability scoresrisk_analysis.py- Risk exposure predictionReport Agent (
report_agent.py):EngineeringReportAgent- ReACT pattern agent for engineering reportsGraphToolsServiceTools Service (
tools.py):EngineeringToolsService- Analysis tool collectionanalyze_quote_accuracy(),identify_process_bottlenecks(),evaluate_collaboration_effectiveness(), etc.Integration
backend/app/services/__init__.pyto export new classesGraphToolsServiceUsage Example
Testing
Summary by CodeRabbit
New Features
Documentation
Chores