OAPE-451: Integrate CrewAI for vendor agnostic multi-agent design-to-code workflow#24
Open
anirudhAgniRedhat wants to merge 1 commit intoshiftweek:mainfrom
Open
OAPE-451: Integrate CrewAI for vendor agnostic multi-agent design-to-code workflow#24anirudhAgniRedhat wants to merge 1 commit intoshiftweek:mainfrom
anirudhAgniRedhat wants to merge 1 commit intoshiftweek:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a complete CrewAI-based multi-agent workflow system (OAPE) with dual backend support (CrewAI and Claude SDK), context loading from files and GitHub, four specialized agents, an 11-task pipeline from design to documentation, repository integration for applying generated code, cost estimation, and extensible skills and prompts loading. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant main.py
participant ContextLoader
participant AdapterFactory
participant CrewAIAdapter
participant Agents
participant Tasks
participant RepoApply
User->>main.py: invoke with context/backend/repo args
main.py->>ContextLoader: load_context_from_file/env/ep_url
ContextLoader-->>main.py: ProjectScope
main.py->>AdapterFactory: get_adapter(backend)
AdapterFactory-->>main.py: CrewAIAdapter | ClaudeSDKAdapter
main.py->>CrewAIAdapter: run(scope)
CrewAIAdapter->>Agents: build_agents(repo_layout)
Agents-->>CrewAIAdapter: [SSE, PSE, SQE, TechWriter]
CrewAIAdapter->>Tasks: build_tasks(scope, agents)
Tasks-->>CrewAIAdapter: [Task 1..11]
CrewAIAdapter->>CrewAIAdapter: Crew.kickoff() with tracing
CrewAIAdapter->>Tasks: execute pipeline
Tasks-->>CrewAIAdapter: outputs (design, impl, docs, etc.)
CrewAIAdapter-->>main.py: WorkflowResult
main.py->>RepoApply: apply_to_repo(design, impl, ...)
RepoApply->>RepoApply: create_branch → write files → verify_compile → commit
RepoApply-->>main.py: result
main.py-->>User: print results + artifacts + costs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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 unit tests (beta)
Comment |
anirudhAgniRedhat
force-pushed
the
crewAIAdapter
branch
from
0e07477 to
de13dec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
crewai/llm_vertex.py (1)
39-71: Silence unused-argument lint warnings.Lines 42-45 and
**kwargsare unused; Ruff flags ARG002. A no-op assignment keeps the signature intact while quieting lint.♻️ Suggested tweak
def call( self, messages: Union[str, List[Dict[str, str]]], tools: Optional[List[dict]] = None, callbacks: Optional[List[Any]] = None, available_functions: Optional[Dict[str, Any]] = None, **kwargs: Any, ) -> Union[str, Any]: + _ = (tools, callbacks, available_functions, kwargs) if isinstance(messages, str): messages = [{"role": "user", "content": messages}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/llm_vertex.py` around lines 39 - 71, In the call method of the Vertex LLM wrapper (function call), silence the ARG002 unused-argument warnings by adding no-op references for the unused parameters (tools, callbacks, available_functions and **kwargs) near the start of the function; e.g. assign them to a throwaway variable or use a tuple unpack (for example: _ = tools; _ = callbacks; _ = available_functions; _ = kwargs) so the signature remains unchanged but linter sees them used.crewai/skills_loader.py (1)
22-47: Log skill file read failures instead of silently skipping.The current
except Exception: continuehides permission or encoding issues, making missing skills hard to diagnose.♻️ Suggested logging
-import os +import os +import logging from pathlib import Path from typing import Optional + +logger = logging.getLogger(__name__) @@ - except Exception: - continue + except Exception as exc: + logger.warning("Failed to read skill file %s", skill_file, exc_info=exc) + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/skills_loader.py` around lines 22 - 47, The code in load_skills_context currently swallows all exceptions when reading SKILL.md (except Exception: continue); change this to catch exceptions as e and log the failure (including skill_file / skill_dir name and the exception details) before continuing so permission/encoding errors are visible; use a module logger (e.g., logger = logging.getLogger(__name__)) or the existing project logger, call logger.error("Failed to read SKILL.md for %s: %s", skill_file, e, exc_info=True) around the skill_file.read_text(...) failure and then continue, leaving the rest of load_skills_context, parts accumulation, and return logic unchanged.crewai/adapters/factory.py (1)
19-22: Consider warning or raising on unrecognized backend names.Currently, any unrecognized backend name (e.g., a typo like
"crewaii"or"claude") silently falls through toCrewAIAdapter. This could mask configuration errors.Consider adding a warning for unrecognized values, or explicitly matching
"crewai":♻️ Optional: explicit validation
def get_adapter(backend: Optional[str] = None) -> WorkflowAdapter: name = (backend or os.getenv("OAPE_BACKEND", "crewai")).strip().lower() if name in ("claude-sdk", "claude_sdk", "claudesdk"): return ClaudeSDKAdapter() + if name != "crewai": + import warnings + warnings.warn(f"Unrecognized backend '{name}', defaulting to 'crewai'") return CrewAIAdapter()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/adapters/factory.py` around lines 19 - 22, The backend selection silently falls through to CrewAIAdapter for any unrecognized name; update the factory logic (variable name, function that returns ClaudeSDKAdapter/CrewAIAdapter) to explicitly accept "crewai" as the CrewAI option and handle unknown values by emitting a clear warning or raising a ValueError instead of silently returning CrewAIAdapter—use the project's logger or warnings.warn to log "unrecognized OAPE_BACKEND: {name}, defaulting to crewai" if you choose warning, or raise ValueError("unrecognized OAPE_BACKEND: {name}") to fail fast; keep the existing ClaudeSDKAdapter branch for ("claude-sdk","claude_sdk","claudesdk") and only treat "crewai" (and equivalent) as the explicit CrewAI match.crewai/command_prompts_loader.py (1)
64-67: Consider logging exceptions instead of silently continuing.The bare
except Exceptionwithcontinuesilently swallows all errors when reading command files, which can hide issues like permission errors or encoding problems. Adding logging would aid debugging.♻️ Proposed fix with logging
+import logging + +_logger = logging.getLogger(__name__) + # In load_command_prompts(): try: content = md_file.read_text(encoding="utf-8") - except Exception: + except Exception as e: + _logger.debug("Failed to read %s: %s", md_file, e) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/command_prompts_loader.py` around lines 64 - 67, The loop that reads command files uses a bare except and continues, hiding I/O/encoding errors; replace the silent continue by catching the exception as e and logging it (e.g., using the module logger or python logging) when md_file.read_text(encoding="utf-8") fails, including md_file (or its path) and the exception message so issues like permission or decoding errors are visible; keep the continue behavior after logging if you still want to skip the file.crewai/adapters/crewai_adapter.py (1)
78-115: Avoid registering trace hooks multiple times.
_crewai_trace_capture_hookand_crewai_trace_capture_after_finalizeare called every run. In long-lived processes this can accumulate handlers and duplicate outputs. Add a guard to register once.♻️ Suggested guard
-_last_trace_access_code: str | None = None +_last_trace_access_code: str | None = None +_trace_capture_hook_installed: bool = False +_trace_capture_after_installed: bool = False @@ def _crewai_trace_capture_hook(): """Register for CrewKickoffCompletedEvent to capture trace ID and URL (before finalize clears them).""" - global _last_trace_id, _last_trace_url, _last_trace_access_code + global _last_trace_id, _last_trace_url, _last_trace_access_code, _trace_capture_hook_installed + if _trace_capture_hook_installed: + return + _trace_capture_hook_installed = True @@ def _crewai_trace_capture_after_finalize(crew): """Register handlers that run AFTER the trace listener: print trace at start (so user can watch live) and capture final URL/access_code at end.""" - global _last_trace_id, _last_trace_url, _last_trace_access_code + global _last_trace_id, _last_trace_url, _last_trace_access_code, _trace_capture_after_installed + if _trace_capture_after_installed: + return + _trace_capture_after_installed = TrueAlso applies to: 118-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/adapters/crewai_adapter.py` around lines 78 - 115, Add a one-time registration guard so the event handlers are not re-registered on every run: introduce a module-level boolean (e.g., _trace_capture_hook_registered) and in both _crewai_trace_capture_hook and _crewai_trace_capture_after_finalize check that flag at the top, return immediately if already True, and set it to True only after successfully registering the crewai_event_bus handlers (the `@crewai_event_bus.on`(CrewKickoffCompletedEvent) registration and the corresponding after-finalize registration). This ensures functions like _capture_trace_before_finalize and the after-finalize handler are only bound once during process lifetime.crewai/tasks.py (1)
27-33: Validateagentslist length to avoid silent fallback.Line 27 silently ignores a provided list if it has fewer than 4 agents, which can mask misconfiguration. Consider failing fast.
♻️ Suggested guard
- sse_agent = (agents[0] if agents and len(agents) >= 4 else sse) + if agents is not None and len(agents) < 4: + raise ValueError("agents must contain 4 entries: SSE, PSE, SQE, Technical Writer") + sse_agent = (agents[0] if agents and len(agents) >= 4 else sse) pse_agent = (agents[1] if agents and len(agents) >= 4 else pse) sqe_agent = (agents[2] if agents and len(agents) >= 4 else sqe) tw_agent = (agents[3] if agents and len(agents) >= 4 else technical_writer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crewai/tasks.py` around lines 27 - 33, The code silently falls back when a provided agents list has fewer than 4 entries; add a guard that validates the agents list length and fails fast: if agents is not None and len(agents) < 4 (or != 4 depending on intended contract) raise a ValueError (or custom exception) with a clear message about requiring four agents; after that, assign sse_agent = agents[0], pse_agent = agents[1], sqe_agent = agents[2], tw_agent = agents[3] (remove the ternary fallbacks), and keep the existing include_layout and scope_md logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crewai/adapters/crewai_adapter.py`:
- Around line 231-233: Update the class docstring on CrewAIAdapter to reflect
the current workflow task count: change the phrase "9 sequential tasks" to "11
sequential tasks" in the triple-quoted docstring for the CrewAIAdapter class so
the documentation matches the implemented workflow.
- Around line 200-227: The _crewai_llm_hooks function currently prints LLM
response previews in _log_response even when CREWAI_DEBUG_LLM is not enabled;
update _log_response (and its use of CREWAI_DEBUG_LLM) so that full response or
previews are only printed when CREWAI_DEBUG_LLM is explicitly truthy, and when
the flag is off replace the preview print with a minimal non-sensitive log
(e.g., only the response length or "response suppressed"); refer to the
_crewai_llm_hooks function and the inner _log_response handler and the
CREWAI_DEBUG_LLM environment variable when making the change.
In `@crewai/context.py`:
- Around line 39-50: The to_markdown method ignores the max_extra_context_chars
parameter because it unconditionally assigns max_extra using _scope_limit;
change the assignment so max_extra uses the passed-in max_extra_context_chars
when provided (e.g., max_extra = max_extra_context_chars or
_scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000)), leaving other behavior
intact; update the to_markdown function where max_extra is set and keep
references to _scope_limit and the max_extra variable.
In `@crewai/llm_vertex.py`:
- Around line 15-27: The constructor (__init__) currently uses "temperature or
0.2" which treats an explicit 0.0 as falsy and replaces it; change the
temperature handling in the super().__init__ call to use an explicit None check
(i.e., pass temperature if temperature is not None, otherwise 0.2) so an
explicit temperature=0.0 is honored; update the super().__init__(model=model,
temperature=...) expression in the __init__ method accordingly.
In `@crewai/README.md`:
- Line 271: Update the README table entry that currently says "`tasks.py` |
Builds the 9 tasks with scope and skills context injected." to reflect the
correct pipeline length: change "9 tasks" to "11 tasks" so it matches other
references to 11 tasks elsewhere in the document (e.g., the pipeline description
and lines referencing 11 tasks); ensure the description still mentions scope and
skills context injected and remains grammatically consistent.
- Around line 226-229: Update the fenced code block showing example output so it
includes a language specifier to satisfy markdownlint MD040: change the opening
backticks for the block containing "Trace ID: <uuid>" and "View trace:
https://app.crewai.com/crewai_plus/trace_batches/<uuid>" to use ```text (or
```plaintext) so the block is treated as plain text.
In `@crewai/repo_apply.py`:
- Around line 17-21: agents._get_llm() can return None for the default backend,
causing _get_llm to return None and triggering the "No LLM configured"
short‑circuit in the repo-apply LLM generation/fix paths; update the _get_llm
function so that if agents_get_llm() returns None you either (a) construct and
return a concrete callable LLM wrapper compatible with the module’s synchronous
.call usage (i.e., instantiate the default backend client and wrap it to match
the expected callable interface) or (b) raise a clear, specific exception (e.g.,
ValueError) stating the default backend is unsupported for direct .call usage so
the caller sees a fast, informative failure instead of the misleading "No LLM
configured" path; ensure callers relying on _get_llm (the branches that
currently short‑circuit with "No LLM configured") receive a real callable or a
clear error.
- Around line 163-187: The _run_commands function currently executes
LLM-supplied strings with shell=True which allows arbitrary shell injection;
change it to validate each command against a strict allowlist and invoke
subprocess.run without a shell using a sequence of args instead of a shell
string. Specifically, implement an allowlist of permitted command names/argument
patterns and for each cmd_str in commands: parse it into an arg list (or map
allowed command keys to predefined arg lists), reject anything not matched, then
call subprocess.run with shell=False and the arg list (preserving
cwd/env/capture_output/text/timeout) and return the same error strings on
failure; update the reference to subprocess.run invocation in _run_commands
accordingly.
- Around line 375-389: The _write_files function currently writes repo-relative
paths without validating them; prevent path traversal by resolving each
destination and ensuring it stays inside the repo and does not target .git: for
each item compute rel (as now) then compute dest = (repo / rel).resolve(); if
dest is not under repo.resolve() (e.g., not str(dest).startswith(str(repo) +
os.sep) or repo == dest.parent check) or any part in
dest.relative_to(repo).parts equals ".git", treat it as invalid and skip/return
an error; only call dest.parent.mkdir(...) and dest.write_text(...) after this
containment check so symlinks/.. cannot escape the repo.
In `@crewai/scope_ztwim_upstream_authority.txt`:
- Around line 13-15: In the requirements section fix the spelling mistakes by
replacing "chnages" with "changes" (both occurrences), "anu" with "any",
"seperate" with "separate", and "Chnages" with "Changes"; update the text
content accordingly so the sentence reads clearly and professionally while
preserving the original meaning and punctuation.
In `@README.md`:
- Around line 50-52: The README section "CrewAI Multi-Agent Workflow" currently
states a 9-task pipeline; update that paragraph (under the "CrewAI Multi-Agent
Workflow" header in README.md) to list the correct 11-task pipeline to match
crewai/README.md and the PR objectives: "design → design review → test plan →
implementation outline → unit tests (SQE) → implementation (SSE) → quality →
code review → address review → write-up → customer doc", and ensure mention of
the SQE and SSE explicit tasks and their mappings to the four agents and skills
directory (plugins/oape/skills/) remains consistent.
---
Nitpick comments:
In `@crewai/adapters/crewai_adapter.py`:
- Around line 78-115: Add a one-time registration guard so the event handlers
are not re-registered on every run: introduce a module-level boolean (e.g.,
_trace_capture_hook_registered) and in both _crewai_trace_capture_hook and
_crewai_trace_capture_after_finalize check that flag at the top, return
immediately if already True, and set it to True only after successfully
registering the crewai_event_bus handlers (the
`@crewai_event_bus.on`(CrewKickoffCompletedEvent) registration and the
corresponding after-finalize registration). This ensures functions like
_capture_trace_before_finalize and the after-finalize handler are only bound
once during process lifetime.
In `@crewai/adapters/factory.py`:
- Around line 19-22: The backend selection silently falls through to
CrewAIAdapter for any unrecognized name; update the factory logic (variable
name, function that returns ClaudeSDKAdapter/CrewAIAdapter) to explicitly accept
"crewai" as the CrewAI option and handle unknown values by emitting a clear
warning or raising a ValueError instead of silently returning CrewAIAdapter—use
the project's logger or warnings.warn to log "unrecognized OAPE_BACKEND: {name},
defaulting to crewai" if you choose warning, or raise ValueError("unrecognized
OAPE_BACKEND: {name}") to fail fast; keep the existing ClaudeSDKAdapter branch
for ("claude-sdk","claude_sdk","claudesdk") and only treat "crewai" (and
equivalent) as the explicit CrewAI match.
In `@crewai/command_prompts_loader.py`:
- Around line 64-67: The loop that reads command files uses a bare except and
continues, hiding I/O/encoding errors; replace the silent continue by catching
the exception as e and logging it (e.g., using the module logger or python
logging) when md_file.read_text(encoding="utf-8") fails, including md_file (or
its path) and the exception message so issues like permission or decoding errors
are visible; keep the continue behavior after logging if you still want to skip
the file.
In `@crewai/llm_vertex.py`:
- Around line 39-71: In the call method of the Vertex LLM wrapper (function
call), silence the ARG002 unused-argument warnings by adding no-op references
for the unused parameters (tools, callbacks, available_functions and **kwargs)
near the start of the function; e.g. assign them to a throwaway variable or use
a tuple unpack (for example: _ = tools; _ = callbacks; _ = available_functions;
_ = kwargs) so the signature remains unchanged but linter sees them used.
In `@crewai/skills_loader.py`:
- Around line 22-47: The code in load_skills_context currently swallows all
exceptions when reading SKILL.md (except Exception: continue); change this to
catch exceptions as e and log the failure (including skill_file / skill_dir name
and the exception details) before continuing so permission/encoding errors are
visible; use a module logger (e.g., logger = logging.getLogger(__name__)) or the
existing project logger, call logger.error("Failed to read SKILL.md for %s: %s",
skill_file, e, exc_info=True) around the skill_file.read_text(...) failure and
then continue, leaving the rest of load_skills_context, parts accumulation, and
return logic unchanged.
In `@crewai/tasks.py`:
- Around line 27-33: The code silently falls back when a provided agents list
has fewer than 4 entries; add a guard that validates the agents list length and
fails fast: if agents is not None and len(agents) < 4 (or != 4 depending on
intended contract) raise a ValueError (or custom exception) with a clear message
about requiring four agents; after that, assign sse_agent = agents[0], pse_agent
= agents[1], sqe_agent = agents[2], tw_agent = agents[3] (remove the ternary
fallbacks), and keep the existing include_layout and scope_md logic unchanged.
Comment on lines
+200
to
+227
| def _crewai_llm_hooks(): | ||
| """Register CrewAI before/after LLM hooks. Log each task once, then short lines for later calls.""" | ||
| from crewai.hooks import after_llm_call, before_llm_call | ||
|
|
||
| _logged_task_keys = set() | ||
|
|
||
| @before_llm_call | ||
| def _log_request(context): | ||
| task_id = getattr(context.task, "id", None) or id(context.task) | ||
| key = (task_id, context.agent.role) | ||
| if key not in _logged_task_keys: | ||
| _logged_task_keys.add(key) | ||
| task_desc = (context.task.description or "")[:80].replace("\n", " ") | ||
| print(f"\n[LLM] → Agent: {context.agent.role} | Task: {task_desc}...") | ||
| else: | ||
| print(f"\n[LLM] → Agent: {context.agent.role} | (same task) iter {context.iterations} msgs {len(context.messages)}") | ||
| return None | ||
|
|
||
| @after_llm_call | ||
| def _log_response(context): | ||
| if context.response is None: | ||
| return None | ||
| n = len(context.response) | ||
| if os.getenv("CREWAI_DEBUG_LLM", "").strip().lower() in ("1", "true", "yes"): | ||
| print(f"\n[LLM] ← Response ({n} chars):\n{context.response}\n") | ||
| else: | ||
| preview = (context.response[:300] + "...") if n > 300 else context.response | ||
| print(f"\n[LLM] ← Response ({n} chars): {preview}") |
Contributor
There was a problem hiding this comment.
Gate LLM response logging behind a debug flag.
Line 223 logs response previews even when debugging is off, which can leak sensitive content to stdout/logs by default. Consider enabling logging only when an explicit debug flag is set.
🔐 Suggested change (log only when debug is enabled)
def _crewai_llm_hooks():
"""Register CrewAI before/after LLM hooks. Log each task once, then short lines for later calls."""
- from crewai.hooks import after_llm_call, before_llm_call
+ if os.getenv("CREWAI_DEBUG_LLM", "").strip().lower() not in ("1", "true", "yes"):
+ return
+ from crewai.hooks import after_llm_call, before_llm_call
@@
- if os.getenv("CREWAI_DEBUG_LLM", "").strip().lower() in ("1", "true", "yes"):
- print(f"\n[LLM] ← Response ({n} chars):\n{context.response}\n")
- else:
- preview = (context.response[:300] + "...") if n > 300 else context.response
- print(f"\n[LLM] ← Response ({n} chars): {preview}")
+ print(f"\n[LLM] ← Response ({n} chars):\n{context.response}\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/adapters/crewai_adapter.py` around lines 200 - 227, The
_crewai_llm_hooks function currently prints LLM response previews in
_log_response even when CREWAI_DEBUG_LLM is not enabled; update _log_response
(and its use of CREWAI_DEBUG_LLM) so that full response or previews are only
printed when CREWAI_DEBUG_LLM is explicitly truthy, and when the flag is off
replace the preview print with a minimal non-sensitive log (e.g., only the
response length or "response suppressed"); refer to the _crewai_llm_hooks
function and the inner _log_response handler and the CREWAI_DEBUG_LLM
environment variable when making the change.
Comment on lines
+231
to
+233
| class CrewAIAdapter(WorkflowAdapter): | ||
| """Execute the OAPE workflow using CrewAI (4 agents, 9 sequential tasks).""" | ||
|
|
Contributor
There was a problem hiding this comment.
Update the task count in the class docstring.
Line 231 still says “9 sequential tasks” but the workflow now has 11.
✏️ Suggested fix
-class CrewAIAdapter(WorkflowAdapter):
- """Execute the OAPE workflow using CrewAI (4 agents, 9 sequential tasks)."""
+class CrewAIAdapter(WorkflowAdapter):
+ """Execute the OAPE workflow using CrewAI (4 agents, 11 sequential tasks)."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/adapters/crewai_adapter.py` around lines 231 - 233, Update the class
docstring on CrewAIAdapter to reflect the current workflow task count: change
the phrase "9 sequential tasks" to "11 sequential tasks" in the triple-quoted
docstring for the CrewAIAdapter class so the documentation matches the
implemented workflow.
Comment on lines
+39
to
+50
| def to_markdown( | ||
| self, | ||
| max_scope_chars: Optional[int] = None, | ||
| max_repo_layout_chars: Optional[int] = None, | ||
| max_extra_context_chars: int = 4000, | ||
| include_repo_layout: bool = True, | ||
| ) -> str: | ||
| """Format scope for injection into task descriptions. Truncated to stay under token limits. | ||
| Set include_repo_layout=False when repo layout is injected in agent backstory instead.""" | ||
| max_scope_chars = max_scope_chars or _scope_limit("OAPE_SCOPE_MAX_CHARS", 12000) | ||
| max_repo_layout_chars = max_repo_layout_chars or _scope_limit("OAPE_REPO_LAYOUT_MAX_CHARS", 6000) | ||
| max_extra = _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000) |
Contributor
There was a problem hiding this comment.
max_extra_context_chars parameter is ignored.
Line 43 exposes max_extra_context_chars, but Line 50 always uses the env default. Callers can’t override the limit as intended.
🐛 Suggested fix
- max_extra = _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000)
+ max_extra = max_extra_context_chars or _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000)📝 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 to_markdown( | |
| self, | |
| max_scope_chars: Optional[int] = None, | |
| max_repo_layout_chars: Optional[int] = None, | |
| max_extra_context_chars: int = 4000, | |
| include_repo_layout: bool = True, | |
| ) -> str: | |
| """Format scope for injection into task descriptions. Truncated to stay under token limits. | |
| Set include_repo_layout=False when repo layout is injected in agent backstory instead.""" | |
| max_scope_chars = max_scope_chars or _scope_limit("OAPE_SCOPE_MAX_CHARS", 12000) | |
| max_repo_layout_chars = max_repo_layout_chars or _scope_limit("OAPE_REPO_LAYOUT_MAX_CHARS", 6000) | |
| max_extra = _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000) | |
| def to_markdown( | |
| self, | |
| max_scope_chars: Optional[int] = None, | |
| max_repo_layout_chars: Optional[int] = None, | |
| max_extra_context_chars: int = 4000, | |
| include_repo_layout: bool = True, | |
| ) -> str: | |
| """Format scope for injection into task descriptions. Truncated to stay under token limits. | |
| Set include_repo_layout=False when repo layout is injected in agent backstory instead.""" | |
| max_scope_chars = max_scope_chars or _scope_limit("OAPE_SCOPE_MAX_CHARS", 12000) | |
| max_repo_layout_chars = max_repo_layout_chars or _scope_limit("OAPE_REPO_LAYOUT_MAX_CHARS", 6000) | |
| max_extra = max_extra_context_chars or _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000) |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 43-43: Unused method argument: max_extra_context_chars
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/context.py` around lines 39 - 50, The to_markdown method ignores the
max_extra_context_chars parameter because it unconditionally assigns max_extra
using _scope_limit; change the assignment so max_extra uses the passed-in
max_extra_context_chars when provided (e.g., max_extra = max_extra_context_chars
or _scope_limit("OAPE_EXTRA_CONTEXT_MAX_CHARS", 4000)), leaving other behavior
intact; update the to_markdown function where max_extra is set and keep
references to _scope_limit and the max_extra variable.
Comment on lines
+15
to
+27
| def __init__( | ||
| self, | ||
| model: str, | ||
| project_id: str, | ||
| region: str, | ||
| temperature: Optional[float] = None, | ||
| max_tokens: int = 8192, | ||
| ): | ||
| super().__init__(model=model, temperature=temperature or 0.2) | ||
| self._project_id = project_id | ||
| self._region = region | ||
| self._max_tokens = max_tokens | ||
| self._client = None |
Contributor
There was a problem hiding this comment.
Honor explicit temperature=0.0.
Line 23 uses temperature or 0.2, which overrides a deliberate zero. Use a None check instead.
🐛 Proposed fix
- super().__init__(model=model, temperature=temperature or 0.2)
+ super().__init__(model=model, temperature=0.2 if temperature is None else temperature)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/llm_vertex.py` around lines 15 - 27, The constructor (__init__)
currently uses "temperature or 0.2" which treats an explicit 0.0 as falsy and
replaces it; change the temperature handling in the super().__init__ call to use
an explicit None check (i.e., pass temperature if temperature is not None,
otherwise 0.2) so an explicit temperature=0.0 is honored; update the
super().__init__(model=model, temperature=...) expression in the __init__ method
accordingly.
Comment on lines
+226
to
+229
| ``` | ||
| Trace ID: <uuid> | ||
| View trace: https://app.crewai.com/crewai_plus/trace_batches/<uuid> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifier to fenced code block.
The code block is missing a language specifier, which triggers a markdownlint warning (MD040). Since this shows example output, use text or plaintext.
📝 Proposed fix
- ```
+ ```text
Trace ID: <uuid>
View trace: https://app.crewai.com/crewai_plus/trace_batches/<uuid>
```📝 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
| ``` | |
| Trace ID: <uuid> | |
| View trace: https://app.crewai.com/crewai_plus/trace_batches/<uuid> | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 226-226: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/README.md` around lines 226 - 229, Update the fenced code block
showing example output so it includes a language specifier to satisfy
markdownlint MD040: change the opening backticks for the block containing "Trace
ID: <uuid>" and "View trace:
https://app.crewai.com/crewai_plus/trace_batches/<uuid>" to use ```text (or
```plaintext) so the block is treated as plain text.
Comment on lines
+17
to
+21
| def _get_llm(): | ||
| """Use same LLM as agents (Vertex or OpenAI) for code generation.""" | ||
| from agents import _get_llm as agents_get_llm | ||
| return agents_get_llm() | ||
|
|
Contributor
There was a problem hiding this comment.
Repo-apply LLM path is disabled when the default backend is used.
agents._get_llm() returns None for the default backend, but this module expects a callable LLM instance. As a result, Lines 151–158 and 417–420 short‑circuit LLM generation/fix paths and show a misleading “No LLM configured” message even when a default backend is configured. Consider returning a concrete LLM instance here or failing fast with a clear error that the default backend isn’t supported for direct .call usage.
Also applies to: 151-158, 417-420
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/repo_apply.py` around lines 17 - 21, agents._get_llm() can return None
for the default backend, causing _get_llm to return None and triggering the "No
LLM configured" short‑circuit in the repo-apply LLM generation/fix paths; update
the _get_llm function so that if agents_get_llm() returns None you either (a)
construct and return a concrete callable LLM wrapper compatible with the
module’s synchronous .call usage (i.e., instantiate the default backend client
and wrap it to match the expected callable interface) or (b) raise a clear,
specific exception (e.g., ValueError) stating the default backend is unsupported
for direct .call usage so the caller sees a fast, informative failure instead of
the misleading "No LLM configured" path; ensure callers relying on _get_llm (the
branches that currently short‑circuit with "No LLM configured") receive a real
callable or a clear error.
Comment on lines
+163
to
+187
| def _run_commands(repo_path: str, commands: list[str]) -> tuple[bool, str]: | ||
| """Run shell commands from repo root. Returns (success, message). Uses same env as go build.""" | ||
| repo = Path(repo_path).resolve() | ||
| if not repo.is_dir(): | ||
| return False, f"Repo path is not a directory: {repo}" | ||
| env = _go_build_env() | ||
| for cmd_str in commands: | ||
| if not cmd_str: | ||
| continue | ||
| try: | ||
| r = subprocess.run( | ||
| cmd_str, | ||
| cwd=repo, | ||
| shell=True, | ||
| env=env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) | ||
| if r.returncode != 0: | ||
| return False, f"Command failed: {cmd_str}\n{(r.stderr or r.stdout or '').strip()}" | ||
| except subprocess.TimeoutExpired: | ||
| return False, f"Command timed out: {cmd_str}" | ||
| except Exception as e: | ||
| return False, f"Command error ({cmd_str}): {e}" |
Contributor
There was a problem hiding this comment.
Do not run LLM-supplied shell commands with shell=True.
Line 173 executes LLM-provided strings in a shell. This enables arbitrary command execution. Restrict to an allowlist and run without a shell.
🔒 Suggested fix (allowlist + no shell)
-import json
+import json
+import shlex
@@
def _run_commands(repo_path: str, commands: list[str]) -> tuple[bool, str]:
@@
- try:
- r = subprocess.run(
- cmd_str,
- cwd=repo,
- shell=True,
- env=env,
- capture_output=True,
- text=True,
- timeout=120,
- )
+ try:
+ cmd = shlex.split(cmd_str)
+ normalized = " ".join(cmd)
+ if normalized not in {"go mod vendor", "go mod tidy"}:
+ return False, f"Command not allowed: {cmd_str}"
+ r = subprocess.run(
+ cmd,
+ cwd=repo,
+ shell=False,
+ env=env,
+ capture_output=True,
+ text=True,
+ timeout=120,
+ )
if r.returncode != 0:
return False, f"Command failed: {cmd_str}\n{(r.stderr or r.stdout or '').strip()}"📝 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 _run_commands(repo_path: str, commands: list[str]) -> tuple[bool, str]: | |
| """Run shell commands from repo root. Returns (success, message). Uses same env as go build.""" | |
| repo = Path(repo_path).resolve() | |
| if not repo.is_dir(): | |
| return False, f"Repo path is not a directory: {repo}" | |
| env = _go_build_env() | |
| for cmd_str in commands: | |
| if not cmd_str: | |
| continue | |
| try: | |
| r = subprocess.run( | |
| cmd_str, | |
| cwd=repo, | |
| shell=True, | |
| env=env, | |
| capture_output=True, | |
| text=True, | |
| timeout=120, | |
| ) | |
| if r.returncode != 0: | |
| return False, f"Command failed: {cmd_str}\n{(r.stderr or r.stdout or '').strip()}" | |
| except subprocess.TimeoutExpired: | |
| return False, f"Command timed out: {cmd_str}" | |
| except Exception as e: | |
| return False, f"Command error ({cmd_str}): {e}" | |
| def _run_commands(repo_path: str, commands: list[str]) -> tuple[bool, str]: | |
| """Run shell commands from repo root. Returns (success, message). Uses same env as go build.""" | |
| repo = Path(repo_path).resolve() | |
| if not repo.is_dir(): | |
| return False, f"Repo path is not a directory: {repo}" | |
| env = _go_build_env() | |
| for cmd_str in commands: | |
| if not cmd_str: | |
| continue | |
| try: | |
| cmd = shlex.split(cmd_str) | |
| normalized = " ".join(cmd) | |
| if normalized not in {"go mod vendor", "go mod tidy"}: | |
| return False, f"Command not allowed: {cmd_str}" | |
| r = subprocess.run( | |
| cmd, | |
| cwd=repo, | |
| shell=False, | |
| env=env, | |
| capture_output=True, | |
| text=True, | |
| timeout=120, | |
| ) | |
| if r.returncode != 0: | |
| return False, f"Command failed: {cmd_str}\n{(r.stderr or r.stdout or '').strip()}" | |
| except subprocess.TimeoutExpired: | |
| return False, f"Command timed out: {cmd_str}" | |
| except Exception as e: | |
| return False, f"Command error ({cmd_str}): {e}" |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 173-173: subprocess call with shell=True identified, security issue
(S602)
[warning] 186-186: 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 `@crewai/repo_apply.py` around lines 163 - 187, The _run_commands function
currently executes LLM-supplied strings with shell=True which allows arbitrary
shell injection; change it to validate each command against a strict allowlist
and invoke subprocess.run without a shell using a sequence of args instead of a
shell string. Specifically, implement an allowlist of permitted command
names/argument patterns and for each cmd_str in commands: parse it into an arg
list (or map allowed command keys to predefined arg lists), reject anything not
matched, then call subprocess.run with shell=False and the arg list (preserving
cwd/env/capture_output/text/timeout) and return the same error strings on
failure; update the reference to subprocess.run invocation in _run_commands
accordingly.
Comment on lines
+375
to
+389
| def _write_files(repo_path: str, files: list[dict[str, str]]) -> tuple[bool, str, int]: | ||
| """Write a list of {path, content} to repo. Returns (success, message, count).""" | ||
| repo = Path(repo_path).resolve() | ||
| written = 0 | ||
| for item in files: | ||
| rel = (item.get("path") or "").strip().lstrip("/") | ||
| content = item.get("content") or "" | ||
| if not rel: | ||
| continue | ||
| dest = repo / rel | ||
| try: | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_text(content, encoding="utf-8") | ||
| written += 1 | ||
| except Exception as e: |
Contributor
There was a problem hiding this comment.
Prevent path traversal when writing LLM-provided files.
Line 384 uses a repo-relative path without checking for .. or symlink escape. A crafted path can write outside the repo (or into .git).
🛡️ Suggested fix (enforce repo boundary)
- dest = repo / rel
+ dest = (repo / rel).resolve()
+ try:
+ dest.relative_to(repo)
+ except ValueError:
+ return False, f"Refusing to write outside repo: {rel}", written
+ if ".git" in dest.parts:
+ return False, f"Refusing to write into .git: {rel}", written
try:
dest.parent.mkdir(parents=True, exist_ok=True)
dest.write_text(content, encoding="utf-8")🧰 Tools
🪛 Ruff (0.15.1)
[warning] 389-389: 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 `@crewai/repo_apply.py` around lines 375 - 389, The _write_files function
currently writes repo-relative paths without validating them; prevent path
traversal by resolving each destination and ensuring it stays inside the repo
and does not target .git: for each item compute rel (as now) then compute dest =
(repo / rel).resolve(); if dest is not under repo.resolve() (e.g., not
str(dest).startswith(str(repo) + os.sep) or repo == dest.parent check) or any
part in dest.relative_to(repo).parts equals ".git", treat it as invalid and
skip/return an error; only call dest.parent.mkdir(...) and dest.write_text(...)
after this containment check so symlinks/.. cannot escape the repo.
Comment on lines
+13
to
+15
| - The API chnages should be part of SpireServer CRD Add the changes in ztwim-repo/api/v1alpha1/spire_server_config_types.go file only do not overwrite anu existing API chnages. | ||
| - DO NOT ADD any seperate condition API chnages for status sub resource. Use the existing conditions if there is any failure to report. | ||
| - Do NOT WRITE tests for the API Chnages so far. |
Contributor
There was a problem hiding this comment.
Correct spelling errors in the requirements section.
Multiple spelling errors reduce document clarity and professionalism:
- Line 13: "chnages" → "changes" (2 occurrences), "anu" → "any"
- Line 14: "seperate" → "separate", "chnages" → "changes"
- Line 15: "Chnages" → "Changes"
📝 Proposed fix for spelling errors
-- The API chnages should be part of SpireServer CRD Add the changes in ztwim-repo/api/v1alpha1/spire_server_config_types.go file only do not overwrite anu existing API chnages.
-- DO NOT ADD any seperate condition API chnages for status sub resource. Use the existing conditions if there is any failure to report.
-- Do NOT WRITE tests for the API Chnages so far.
+- The API changes should be part of SpireServer CRD Add the changes in ztwim-repo/api/v1alpha1/spire_server_config_types.go file only do not overwrite any existing API changes.
+- DO NOT ADD any separate condition API changes for status sub resource. Use the existing conditions if there is any failure to report.
+- Do NOT WRITE tests for the API Changes so far.📝 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
| - The API chnages should be part of SpireServer CRD Add the changes in ztwim-repo/api/v1alpha1/spire_server_config_types.go file only do not overwrite anu existing API chnages. | |
| - DO NOT ADD any seperate condition API chnages for status sub resource. Use the existing conditions if there is any failure to report. | |
| - Do NOT WRITE tests for the API Chnages so far. | |
| - The API changes should be part of SpireServer CRD Add the changes in ztwim-repo/api/v1alpha1/spire_server_config_types.go file only do not overwrite any existing API changes. | |
| - DO NOT ADD any separate condition API changes for status sub resource. Use the existing conditions if there is any failure to report. | |
| - Do NOT WRITE tests for the API Changes so far. |
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Ensure spelling is correct
Context: ...(cert-manager, SPIRE, Vault). - The API chnages should be part of SpireServer CRD Add t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~13-~13: Ensure spelling is correct
Context: ...fig_types.go file only do not overwrite anu existing API chnages. - DO NOT ADD any ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~14-~14: Ensure spelling is correct
Context: ...- DO NOT ADD any seperate condition API chnages for status sub resource. Use the existi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~15-~15: Ensure spelling is correct
Context: ...eport. - Do NOT WRITE tests for the API Chnages so far. - Scope of operator is not to c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crewai/scope_ztwim_upstream_authority.txt` around lines 13 - 15, In the
requirements section fix the spelling mistakes by replacing "chnages" with
"changes" (both occurrences), "anu" with "any", "seperate" with "separate", and
"Chnages" with "Changes"; update the text content accordingly so the sentence
reads clearly and professionally while preserving the original meaning and
punctuation.
Comment on lines
+50
to
+52
| ## CrewAI Multi-Agent Workflow | ||
|
|
||
| A **project-agnostic** CrewAI setup lives in **[crewai/](crewai/)**. It runs a 9-task pipeline (design → design review → test cases → implementation outline → quality → code review → address review → write-up → customer doc) with four agents (SSE, PSE, SQE, Technical Writer). Agents take learnings from **skills** in `plugins/oape/skills/` (e.g. Effective Go). Scope is set at runtime via env or CLI—no project-specific context. See [crewai/README.md](crewai/README.md) for setup and usage. |
Contributor
There was a problem hiding this comment.
Documentation inconsistency: 9-task vs 11-task pipeline.
This section describes a "9-task pipeline," but the PR objectives and crewai/README.md (Line 7) describe an "11-task pipeline" that includes explicit unit tests (SQE) and implementation (SSE) tasks. The task list here is also missing those two tasks.
Consider updating to align with the actual pipeline:
design → design review → test plan → implementation outline → unit tests (SQE) → implementation (SSE) → quality → code review → address review → write-up → customer doc
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 50 - 52, The README section "CrewAI Multi-Agent
Workflow" currently states a 9-task pipeline; update that paragraph (under the
"CrewAI Multi-Agent Workflow" header in README.md) to list the correct 11-task
pipeline to match crewai/README.md and the PR objectives: "design → design
review → test plan → implementation outline → unit tests (SQE) → implementation
(SSE) → quality → code review → address review → write-up → customer doc", and
ensure mention of the SQE and SSE explicit tasks and their mappings to the four
agents and skills directory (plugins/oape/skills/) remains consistent.
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
1 participant
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
Integrates CrewAI as an OAPE workflow backend so a single run can go from a feature scope to design, review, test plan, implementation outline, unit tests, implementation code, and customer docs.
Highlights
go build(ormake build) check, and commit; on vendoring errors the agent can suggestgo mod vendorand the pipeline runs it.plugins/oape/skills/.Testing
python main.py(default scope).--repo-path <path> --apply-to-repo; use--no-apply-to-repoto skip apply../scripts/test_crewai.sh [smoke|context|output-dir|apply].Summary by CodeRabbit
New Features
Documentation