Conversation
|
|
📝 WalkthroughWalkthroughAdds a complete Finance Insight sample: a Python/Flask async job API with in-memory job store and cleanup, a CrewAI four-agent workflow, new data/tools (Alpha Vantage, Twelve Data, SerpAPI, sandboxed Python exec), packaging/Docker and YAML configs, plus a Next.js TypeScript frontend with components, client API, and styling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as "Next.js Frontend"
participant API as "Flask API Server"
participant Crew as "CrewAI Crew"
participant Tools as "Data Tools"
User->>Frontend: submit request
Frontend->>API: POST /chat/async (payload)
API-->>Frontend: { jobId }
loop poll
Frontend->>API: GET /chat/async/{jobId}/status
API-->>Frontend: status + traces
end
API->>Crew: kickoff job (RUNNING)
Crew->>Tools: invoke researcher/quant/auditor/reporter tools
Tools-->>Crew: data / computations / audit results
Crew-->>API: final report (COMPLETED)
Frontend->>API: GET /chat/async/{jobId}/result
API-->>Frontend: { result.report }
Frontend->>User: render report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive finance-insight-agent sample demonstrating an AI-powered financial research assistant. The sample integrates CrewAI for agent orchestration, multiple financial data APIs (SerpAPI, Twelve Data, Alpha Vantage), and provides both a Flask backend and Next.js frontend.
Changes:
- Backend API server with async job processing and event tracing
- Four specialized agents (Researcher, Quant, Auditor, Reporter) with custom tools
- Next.js UI with dark/light mode, settings management, and real-time job status
- Docker deployment configuration and AMP/Choreo integration guide
Reviewed changes
Copilot reviewed 35 out of 102 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| samples/finance-insight-agent/src/finance_insight_service/api_server.py | Flask server with async job management, CORS, and authentication |
| samples/finance-insight-agent/src/finance_insight_service/crew.py | CrewAI orchestration with 4 agents and sequential task execution |
| samples/finance-insight-agent/src/finance_insight_service/tools/*.py | Custom tools for news search, price history, fundamentals, and Python execution |
| samples/finance-insight-agent/src/ui/lib/api.ts | Frontend API client with polling and job cancellation |
| samples/finance-insight-agent/src/ui/app/components/*.tsx | React components for chat interface, settings, and theme toggle |
| samples/finance-insight-agent/Dockerfile | Python 3.11 container with uv package manager |
| samples/finance-insight-agent/pyproject.toml | Project dependencies and console script configuration |
| samples/finance-insight-agent/README.md | Architecture overview and AMP deployment instructions |
| samples/finance-insight-agent/API_KEYS.md | Detailed API key acquisition guide |
| samples/finance-insight-agent/.env.example | Environment variable template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const DEFAULT_API_BASE_URL = | ||
| process.env.NEXT_PUBLIC_API_BASE_URL ?? "http://default.localhost:9080/finance-insight"; |
There was a problem hiding this comment.
The DEFAULT_API_BASE_URL points to "http://default.localhost:9080/finance-insight" which uses HTTP instead of HTTPS and includes a non-standard hostname "default.localhost". For a production sample, this should use a more typical default like "http://localhost:5000" or document that this is AMP/Choreo-specific and needs to be overridden in .env.local for local development.
| const trimmedKey = apiKey.trim(); | ||
|
|
||
| setApiConfig({ baseUrl: trimmedBase, apiKey: trimmedKey }); | ||
| localStorage.setItem("financeApiKeys", JSON.stringify(keys)); |
There was a problem hiding this comment.
API keys are stored in localStorage which is vulnerable to XSS attacks. While the comment acknowledges "Keys are stored locally in your browser" and mentions production needs server-side auth, storing sensitive API keys in localStorage is insecure. Consider using httpOnly cookies or warning users more prominently that this is for development only, not production.
| try: | ||
| with redirect_stdout(stdout): | ||
| exec(code_to_run, context) | ||
| except Exception as exc: | ||
| return json.dumps( | ||
| { | ||
| "status": "CODE_ERROR", | ||
| "error": f"execution failed: {exc}", | ||
| "traceback": traceback.format_exc(), | ||
| "code": code_to_run, | ||
| }, | ||
| ensure_ascii=True, | ||
| ) |
There was a problem hiding this comment.
The safe_python_exec tool uses exec() to execute arbitrary Python code. While there are restrictions on imports and builtins, the tool is still vulnerable to resource exhaustion attacks (infinite loops, memory bombs). Consider adding timeout limits, memory limits, and CPU time restrictions using resource limits or a process sandbox.
| if not symbol: | ||
| return _error_payload("symbol is required") | ||
|
|
||
| api_key = (os.getenv("ALPHAVANTAGE_API_KEY") or "").strip().strip("\"'").strip() |
There was a problem hiding this comment.
Missing input validation for the Alpha Vantage API key. The strip() operations with chained methods on line 31 can fail if ALPHAVANTAGE_API_KEY is None. Consider adding a null check before the strip chain or using a safer pattern like: api_key = (os.getenv("ALPHAVANTAGE_API_KEY") or "").strip().strip("\"'").strip().
| .pnpm-debug.log* | ||
|
|
||
| # env files (can opt-in for committing if needed) | ||
| .env* |
There was a problem hiding this comment.
The .env file is excluded from git (line 34) which is correct, but the .gitignore uses dotenv* pattern which would also exclude legitimate files like .env.example or .env.template if they existed. Consider being more specific with the pattern, such as .env and .env.local but not .env.*, or explicitly whitelist .env.example with !.env.example.
| .env* | |
| .env | |
| .env.local | |
| .env.development.local | |
| .env.test.local | |
| .env.production.local |
| # In-memory job storage (for production, use Redis or database) | ||
| jobs = {} | ||
| jobs_lock = threading.Lock() |
There was a problem hiding this comment.
The in-memory jobs dictionary with threading.Lock is not suitable for production deployments with multiple processes or servers. The comment acknowledges this ("for production, use Redis or database"), but a sample should demonstrate production-ready patterns. Consider including a simple Redis implementation with a fallback to in-memory for development, or documenting this as a development-only limitation more prominently.
| import ast | ||
| import datetime as datetime_module | ||
| import io | ||
| import json |
There was a problem hiding this comment.
Module 'json' is imported with both 'import' and 'import from'.
| for unsub in unsubscribe_funcs: | ||
| try: | ||
| unsub() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| normalized.append(json.loads(text)) | ||
| continue | ||
| except JSONDecodeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except JSONDecodeError: | |
| except JSONDecodeError: | |
| # If the string isn't valid JSON, fall back to other parsing | |
| # strategies below; ultimately we keep the original item. |
| try: | ||
| normalized.append(ast.literal_eval(text)) | ||
| continue | ||
| except (ValueError, SyntaxError): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
![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
🤖 Fix all issues with AI agents
In `@samples/finance-insight-agent/API_KEYS.md`:
- Around line 27-70: Update the SerpAPI and Twelve Data sections to remove
hard-coded quotas and instead link to each provider's pricing/free-tier pages;
specifically, change the SerpAPI free-tier mention under the "SerpAPI Key
(Required)" heading to reflect the current 250 searches/month and add a link to
SerpAPI’s pricing/free-tier page, and update the "Twelve Data API Key (Required
for market data)" section to state that the 800 value refers to credits per day
(not uniform requests) and replace the hard number with a link to Twelve Data’s
pricing/limits documentation so readers can see endpoint-specific credit costs.
In `@samples/finance-insight-agent/Dockerfile`:
- Line 12: The Dockerfile uses a broad COPY . . which can accidentally bake
local secrets (like .env) into the image; create a .dockerignore that excludes
sensitive files and bulky artifacts (e.g., .env, .env.* , .git, node_modules,
build/, *.pem, credentials/*) and then update build steps to copy only required
artifacts or rely on the .dockerignore; reference the COPY . . instruction in
the Dockerfile and add the new .dockerignore to the project to prevent secret
leakage.
In `@samples/finance-insight-agent/pyproject.toml`:
- Around line 19-23: The project uses a src/ layout with the package at
src/finance_insight_service and the entry point
finance_insight_service.api_server:main, but [tool.uv] is missing the
package-dir setting; update the [tool.uv] section to add package-dir = "src" so
UV can resolve the finance_insight_service package for installation and the
finance_insight_api entry point to run correctly.
In `@samples/finance-insight-agent/README.md`:
- Around line 41-44: The bare AMP repo URL in the README (the line "AMP repo:
https://github.com/wso2/ai-agent-management-platform/tree/amp/v0") triggers
MD034; update that line to either wrap the URL in angle brackets (e.g.,
<https://...>) or convert it to a proper markdown link with descriptive text
(e.g., [AMP repo](https://github.com/.../tree/amp/v0)) so the URL is not bare.
In `@samples/finance-insight-agent/src/finance_insight_service/api_server.py`:
- Around line 384-428: Events from concurrent jobs mix because the global
crewai_event_bus handler_wrapper doesn't filter by crew and all jobs use the
same hardcoded crew name; pass job_id into FinanceInsightCrew and use it in
build_crew to create a unique crew name (e.g.,
FinanceInsightCrew(job_id=job_id).build_crew()), then update handler_wrapper to
ignore events where getattr(evt, "crew_name", None) != crew.name so only events
for the current crew are processed; keep references to crewai_event_bus,
handler_wrapper, FinanceInsightCrew, build_crew, and crew.name when making the
change.
In
`@samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py`:
- Around line 126-129: The sandbox exposes dangerous introspection via the
builtins (notably the use of type in the allowed builtins) which allows payloads
like () .__class__.__bases__[0].__subclasses__() to escape; update
safe_python_exec.py by adding a clear security comment above the exec invocation
(and the allowed_builtins definition) stating this sandbox is only for trusted
agent-generated code, why type was included, and the assumptions; for stronger
protection either remove or block type/introspection from the allowed_builtins
used by exec (the context passed to exec/code_to_run/redirect_stdout), or
replace this approach with an AST-level sanitizer (e.g., RestrictedPython) or
run code in a subprocess/container with strict resource and capability
limits—document which option you choose and reference the exec(code_to_run,
context) and allowed_builtins/type symbols so reviewers can verify the change.
In `@samples/finance-insight-agent/src/ui/app/components/SettingsForm.tsx`:
- Around line 40-70: The SettingsForm currently accesses localStorage unguarded
(localStorage.getItem in the mount effect and localStorage.setItem in
handleSave), which can throw in restricted contexts; wrap both usages in
try/catch blocks (or feature-detect window.localStorage) to safely handle
SecurityError/DOMException and fallback silently (or log) without crashing,
updating the code paths in the effect that parses savedKeys and in handleSave
before calling localStorage.setItem("financeApiKeys", JSON.stringify(keys)),
keeping existing behavior for setApiConfig, setKeys, fetchConfig, and
setCapabilities.
In `@samples/finance-insight-agent/src/ui/app/components/TopBar.tsx`:
- Around line 1-28: Add the "use client" directive as the first line of this
module (before any imports) to mark TopBar as a client component; update the
TopBar component file so the "use client" pragma precedes imports and ensure the
file still exports the TopBar function that uses the onNewChat handler and
imports ThemeToggle and Link.
In `@samples/finance-insight-agent/src/ui/app/globals.css`:
- Around line 1583-1589: The .back-link rule uses an invalid camelCase property
"fontSize" so the font size isn't applied; change "fontSize: 14px" to the valid
CSS property "font-size: 14px" inside the .back-link selector to ensure the rule
takes effect.
In `@samples/finance-insight-agent/src/ui/package.json`:
- Line 9: The "lint" npm script in package.json currently runs just "eslint"
with no target patterns, so ESLint won't run any files; update the "lint" script
(the "lint" entry in package.json) to include file globs or use the Next.js
helper (e.g., replace the current "eslint" value with a command that lints your
source files such as specifying src/**/*.{js,jsx,ts,tsx} or .) or switch to
"next lint" for Next.js projects so the linter actually runs; modify the "lint"
script accordingly.
- Around line 12-15: Update the package.json dependency versions for React and
React DOM from "react": "19.2.3" and "react-dom": "19.2.3" to "19.2.4" to apply
the security patch; locate the dependencies block in
samples/finance-insight-agent/src/ui/package.json and change the values for the
"react" and "react-dom" entries accordingly, then run the project’s install
command (e.g., npm install or yarn) and verify builds/tests pass.
🧹 Nitpick comments (14)
samples/finance-insight-agent/src/finance_insight_service/tools/__init__.py (1)
1-1: Valid package marker with room for API improvement.The module docstring is appropriate and the file correctly marks this directory as a Python package.
Consider exposing the tool modules in this
__init__.pyfor a cleaner import experience. Based on the PR context, there are several tool modules (e.g.,company_fundamentals_fetch,price_history_fetch,safe_python_exec,serpapi_news_search) that could be imported here. This would allow users to import directly from the package level:from finance_insight_service.tools import CompanyFundamentalsTool, PriceHistoryToolinstead of:
from finance_insight_service.tools.company_fundamentals_fetch import CompanyFundamentalsTool from finance_insight_service.tools.price_history_fetch import PriceHistoryToolExample implementation
"""Tooling for the finance insight service.""" + +from finance_insight_service.tools.company_fundamentals_fetch import CompanyFundamentalsTool +from finance_insight_service.tools.price_history_fetch import PriceHistoryTool +from finance_insight_service.tools.safe_python_exec import SafePythonExecTool +from finance_insight_service.tools.serpapi_news_search import SerpAPINewsSearchTool + +__all__ = [ + "CompanyFundamentalsTool", + "PriceHistoryTool", + "SafePythonExecTool", + "SerpAPINewsSearchTool", +]Note: Adjust the import names based on the actual class/function names exported by each module.
samples/finance-insight-agent/src/ui/package.json (2)
2-2: Consider a more descriptive package name.The package name "web" is very generic. Since this is the Finance Insight Agent UI, consider a more descriptive name like
finance-insight-uiorfinance-insight-webto better reflect the project's purpose.📝 Suggested improvement
- "name": "web", + "name": "finance-insight-ui",
5-10: Consider adding a type-check script.TypeScript projects typically benefit from a dedicated type-checking script for development and CI/CD workflows, separate from the build process.
📝 Suggested addition
"scripts": { "dev": "next dev", "build": "next build", "start": "next start", - "lint": "eslint" + "lint": "eslint .", + "type-check": "tsc --noEmit" },samples/finance-insight-agent/src/ui/README.md (1)
1-36: Consider adding sample-specific UI notes.
This is the stock Next.js README; a short section on how it connects to the Finance Insight backend (env vars, endpoints, dev workflow) would improve usability.samples/finance-insight-agent/.env.example (1)
8-14: Fix env variable format to avoid parsing issues.The trailing spaces after
=on these lines may cause issues when users fill in values. Standard practice is to use either empty values or quoted placeholders.Proposed fix
# News Search (required) -SERPAPI_API_KEY= # Get from serpapi.com +SERPAPI_API_KEY= # Get from serpapi.com # Market Data (required) -TWELVE_DATA_API_KEY= # Get from twelvedata.com +TWELVE_DATA_API_KEY= # Get from twelvedata.com # Company Fundamentals (required for fundamental analysis) -ALPHAVANTAGE_API_KEY= # Get from alphavantage.co +ALPHAVANTAGE_API_KEY= # Get from alphavantage.cosamples/finance-insight-agent/src/ui/PROJECT_GUIDE.md (1)
44-46: Add language specifier to the fenced code block.The code block is missing a language identifier, which affects syntax highlighting and tooling.
Proposed fix
-``` +```env NEXT_PUBLIC_API_BASE_URL=http://localhost:5000 ```samples/finance-insight-agent/src/finance_insight_service/tools/serpapi_news_search.py (1)
72-72: Consider usingdatetime.now(datetime.timezone.utc)instead ofdatetime.utcnow().
datetime.utcnow()is deprecated in Python 3.12+ and will be removed in a future version. The same pattern appears in_error_payload.♻️ Suggested fix
+from datetime import datetime, timezone + - "fetched_at": datetime.utcnow().isoformat() + "Z", + "fetched_at": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"),samples/finance-insight-agent/src/ui/lib/api.ts (2)
121-188: Add a maximum polling duration to prevent indefinite polling.The
while (true)loop exits only when the job reaches a terminal state or is aborted. If the backend hangs in a non-terminal state, the client will poll indefinitely, consuming resources and potentially confusing users.♻️ Suggested fix
const pollStatus = async (): Promise<ReportResponse> => { + const startTime = Date.now(); + const MAX_POLL_DURATION_MS = 15 * 60 * 1000; // 15 minutes + while (true) { if (signal?.aborted) { throw new DOMException("Request cancelled", "AbortError"); } + if (Date.now() - startTime > MAX_POLL_DURATION_MS) { + throw new Error("Request timed out after 15 minutes"); + } await new Promise(resolve => setTimeout(resolve, 2000));
142-152: Simplify trace deduplication logic.The condition
statusData.traceCount > lastTraceCountis redundant whenseenTracesalready tracks emitted traces. This extra condition could cause missed traces iftraceCountis ever out of sync with actual trace entries.♻️ Suggested simplification
if (onTrace && statusData.traces) { for (const trace of statusData.traces) { const traceKey = trace.seq ? `seq-${trace.seq}` : `${trace.timestamp}-${trace.message}`; - if (!seenTraces.has(traceKey) && statusData.traceCount > lastTraceCount) { + if (!seenTraces.has(traceKey)) { seenTraces.add(traceKey); onTrace(trace.message); } } - lastTraceCount = statusData.traceCount; }samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py (1)
213-245: Indentation normalization is fragile and may corrupt valid code.The heuristic approach doesn't account for:
- Multi-line strings (triple quotes)
- Line continuations (
\or implicit in parentheses)- Decorators
- Complex nested structures
For example, this valid code would be mangled:
def foo(): x = """ indented text """Consider using
textwrap.dedentalone (already applied at line 38) and letting the user provide properly formatted code, or useast.parseto validate syntax before execution.samples/finance-insight-agent/src/finance_insight_service/tools/price_history_fetch.py (2)
86-99: Silent data dropping may mask API issues.Rows with missing or invalid fields are silently skipped. Consider logging a warning or including a count of skipped rows in the response to aid debugging.
♻️ Optional: Track skipped rows
values = payload["values"] data = [] + skipped = 0 for row in values: try: data.append( { "date": row["datetime"], "open": float(row["open"]), "high": float(row["high"]), "low": float(row["low"]), "close": float(row["close"]), "volume": float(row.get("volume") or 0), } ) except (KeyError, ValueError): + skipped += 1 continue data.reverse() return { "provider": "twelve_data", "symbol": symbol, "interval": interval, "fetched_at": datetime.utcnow().isoformat() + "Z", "data": data[-outputsize:], - "error": "", + "error": f"skipped {skipped} malformed rows" if skipped else "", }
45-50: Usedatetime.now(timezone.utc)instead of deprecateddatetime.utcnow().Same deprecation concern as in other tool files. Consider updating for Python 3.12+ compatibility.
Also applies to: 106-106, 117-117
samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.py (2)
37-40: Sequential API calls may be slow; consider documenting or parallelizing.Four sequential API calls with 20s timeouts could take up to 80 seconds in the worst case. While Alpha Vantage's free tier has rate limits (5 calls/minute), the premium tier allows parallel requests.
Consider using
concurrent.futures.ThreadPoolExecutorwith a configurable parallelism setting, or document the expected latency.♻️ Optional: Parallel fetching with ThreadPoolExecutor
+from concurrent.futures import ThreadPoolExecutor, as_completed + def _run(self, symbol: str, limit: int = 1) -> str: # ... validation ... - overview, overview_error = _fetch_alpha("OVERVIEW", symbol, api_key) - income_raw, income_error = _fetch_alpha("INCOME_STATEMENT", symbol, api_key) - balance_raw, balance_error = _fetch_alpha("BALANCE_SHEET", symbol, api_key) - cash_raw, cash_error = _fetch_alpha("CASH_FLOW", symbol, api_key) + functions = ["OVERVIEW", "INCOME_STATEMENT", "BALANCE_SHEET", "CASH_FLOW"] + results = {} + with ThreadPoolExecutor(max_workers=4) as executor: + futures = {executor.submit(_fetch_alpha, fn, symbol, api_key): fn for fn in functions} + for future in as_completed(futures): + fn = futures[future] + results[fn] = future.result() + + overview, overview_error = results["OVERVIEW"] + income_raw, income_error = results["INCOME_STATEMENT"] + balance_raw, balance_error = results["BALANCE_SHEET"] + cash_raw, cash_error = results["CASH_FLOW"]
50-50: Usedatetime.now(timezone.utc)instead of deprecateddatetime.utcnow().Consistent with recommendations for other tool files.
Also applies to: 103-103
| && apt-get install -y --no-install-recommends build-essential libgomp1 \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY . . |
There was a problem hiding this comment.
Prevent accidental secret leakage in the image.
COPY . . will include any local .env or other sensitive files unless a .dockerignore is present, which can bake secrets into the image. Consider adding a .dockerignore for secrets and bulky artifacts.
🛡️ Suggested .dockerignore (new file)
+.env
+.env.*
+__pycache__/
+.venv/
+.pytest_cache/
+*.egg-info/
+data/
+node_modules/
+.next/
+.choreo/
+dist/
+build/🤖 Prompt for AI Agents
In `@samples/finance-insight-agent/Dockerfile` at line 12, The Dockerfile uses a
broad COPY . . which can accidentally bake local secrets (like .env) into the
image; create a .dockerignore that excludes sensitive files and bulky artifacts
(e.g., .env, .env.* , .git, node_modules, build/, *.pem, credentials/*) and then
update build steps to copy only required artifacts or rely on the .dockerignore;
reference the COPY . . instruction in the Dockerfile and add the new
.dockerignore to the project to prevent secret leakage.
| [project.scripts] | ||
| finance_insight_api = "finance_insight_service.api_server:main" | ||
|
|
||
| [tool.uv] | ||
| package = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify package structure and UV configuration for finance-insight-agent
echo "=== Directory structure ==="
tree -L 3 samples/finance-insight-agent/ 2>/dev/null || find samples/finance-insight-agent/ -type f -o -type d | head -30
echo -e "\n=== Check if src layout exists ==="
if [ -d "samples/finance-insight-agent/src" ]; then
echo "src/ directory exists"
ls -la samples/finance-insight-agent/src/
else
echo "src/ directory does not exist"
fi
echo -e "\n=== Check package location ==="
fd "finance_insight_service" samples/finance-insight-agent/ -type d
echo -e "\n=== Check api_server.py and main function ==="
if [ -f "samples/finance-insight-agent/src/finance_insight_service/api_server.py" ]; then
echo "Found at src/finance_insight_service/api_server.py"
grep -n "def main" samples/finance-insight-agent/src/finance_insight_service/api_server.py
elif [ -f "samples/finance-insight-agent/finance_insight_service/api_server.py" ]; then
echo "Found at finance_insight_service/api_server.py"
grep -n "def main" samples/finance-insight-agent/finance_insight_service/api_server.py
else
echo "api_server.py not found in expected locations"
find samples/finance-insight-agent/ -name "api_server.py" 2>/dev/null
fi
echo -e "\n=== Full pyproject.toml content ==="
cat samples/finance-insight-agent/pyproject.tomlRepository: wso2/ai-agent-management-platform
Length of output: 1838
Configure package-dir in [tool.uv] for proper src-layout support.
The src/ layout exists with the package correctly placed at src/finance_insight_service/ and the entry point api_server.py:main is present. However, [tool.uv] lacks explicit package-dir configuration. Add package-dir = "src" to ensure UV correctly resolves the package for installation and entry point execution:
[tool.uv]
package = true
package-dir = "src"
🤖 Prompt for AI Agents
In `@samples/finance-insight-agent/pyproject.toml` around lines 19 - 23, The
project uses a src/ layout with the package at src/finance_insight_service and
the entry point finance_insight_service.api_server:main, but [tool.uv] is
missing the package-dir setting; update the [tool.uv] section to add package-dir
= "src" so UV can resolve the finance_insight_service package for installation
and the finance_insight_api entry point to run correctly.
samples/finance-insight-agent/src/finance_insight_service/api_server.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@samples/finance-insight-agent/API_KEYS.md`:
- Around line 96-98: Replace the insecure example that shows passing secrets via
command-line environment flags (the line starting with `docker run -e
OPENAI_API_KEY=... -e SERPAPI_API_KEY=... -e TWELVE_DATA_API_KEY=... -e
ALPHAVANTAGE_API_KEY=... finance-insight`) with a secure alternative: instruct
users to load env vars from an env-file (mention using `--env-file`), or to use
Docker secrets (create secrets and reference them in compose/swarm), or to mount
secret files into the container (`-v /secure/path/...:/run/secrets/...:ro`);
update the sample and explanatory text to show one of these secure alternatives
and remove the direct `-e` example.
In `@samples/finance-insight-agent/src/finance_insight_service/api_server.py`:
- Around line 72-73: The bare "except:" in
samples/finance-insight-agent/src/finance_insight_service/api_server.py should
be changed to "except Exception:" to avoid catching
SystemExit/KeyboardInterrupt; locate the try/except block (the loop containing
the "except:" clause) and replace the bare except with "except Exception:" and
optionally log the exception or continue, ensuring normal shutdown signals are
not swallowed.
- Around line 70-76: The code creates a timezone-aware datetime by parsing
updated_str with datetime.fromisoformat (stored in updated_time) but compares it
to a naive datetime returned by _utc_now() (which uses datetime.utcnow()),
causing a TypeError; fix by normalizing both to the same kind: either make
_utc_now() return an aware UTC datetime (e.g., use datetime.now(timezone.utc))
or convert updated_time to naive UTC before subtraction (e.g., updated_time =
updated_time.replace(tzinfo=None) or
updated_time.astimezone(timezone.utc).replace(tzinfo=None)); update the
comparison logic around updated_time and the call sites that use _utc_now()
accordingly so both operands are either naive or aware.
In `@samples/finance-insight-agent/src/finance_insight_service/crew.py`:
- Around line 118-121: The context wiring uses a set comparison that ignores
order, so when selected_tasks is a permutation of full_order you may wire
contexts pointing to tasks that will run later; change the guard from
set(selected_tasks) == set(full_order) to an ordered comparison selected_tasks
== full_order (or explicitly check ordering) before assigning contexts to
quant_task, audit_task, and report_task so those context dependencies only get
wired when the execution order matches; alternatively add a clear
docstring/comment on the function explaining custom task ordering is unsupported
when context wiring is applied.
🧹 Nitpick comments (5)
samples/finance-insight-agent/API_KEYS.md (2)
100-107: Add guidance on protecting systemd unit files containing secrets.When embedding secrets in systemd unit files via
Environment=directives, the file permissions should be restricted to prevent unauthorized access.🛡️ Suggested addition
Add a note after the systemd example:
[Service] Environment="OPENAI_API_KEY=sk-..." Environment="SERPAPI_API_KEY=..." Environment="TWELVE_DATA_API_KEY=..." Environment="ALPHAVANTAGE_API_KEY=..." + +# Restrict unit file permissions: +# sudo chmod 600 /etc/systemd/system/finance-insight.service +# Or use EnvironmentFile=/secure/path/finance-insight.env with LoadCredential=
88-90: Consider adding a security note about binding to 0.0.0.0.The development command binds to
0.0.0.0, which exposes the service to all network interfaces. While this is useful for testing across devices or in containers, users should be aware of the security implications.📝 Suggested clarification
The backend must be **restarted** after changing `.env`: ```bash uv run finance_insight_api --host 0.0.0.0 --port 5000 +# Note: --host 0.0.0.0 exposes the service to your network. +# For local-only testing, use --host 127.0.0.1</details> </blockquote></details> <details> <summary>samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py (1)</summary><blockquote> `216-248`: **Indentation heuristics may break valid Python in edge cases.** The `_normalize_indentation` function uses heuristics that can mishandle: - `elif`/`else`/`except`/`finally` at line 234-235 dedent by 4 from `prev_indent`, but should match the originating `if`/`try` statement's indentation level, not just be 4 less than the previous line (e.g., nested conditionals). - Multi-line strings or comments containing code-like patterns. Since this is best-effort normalization for LLM-generated code, consider documenting these limitations or adding a fallback that preserves original indentation if the normalized code fails to parse. </blockquote></details> <details> <summary>samples/finance-insight-agent/src/finance_insight_service/api_server.py (2)</summary><blockquote> `180-182`: **Inconsistent timezone usage: `datetime.now()` vs `_utc_now()`.** Lines 181-182 use `datetime.now()` (local timezone) for `current_date` and `current_year`, while other timestamps in the file use `_utc_now()` (UTC). This could cause confusion when the server runs in different timezones. Consider using `_utc_now()` consistently, or document that `current_date` intentionally reflects local time for user context. --- `280-280`: **Memory metric is misleading: `sys.getsizeof(jobs)` only measures dict overhead.** `sys.getsizeof(jobs)` returns the size of the dictionary object itself (~200 bytes), not the memory consumed by its contents (job data, traces, results). For meaningful metrics, consider using a deep size calculation or simply reporting job count. <details> <summary>♻️ Suggested fix</summary> ```diff - "memory_mb": sys.getsizeof(jobs) / (1024 * 1024), + # Note: This is an approximation based on job count + "estimated_jobs_memory_kb": job_count * 10, # ~10KB per job estimateOr remove the metric entirely since it's not accurate:
- "memory_mb": sys.getsizeof(jobs) / (1024 * 1024),
samples/finance-insight-agent/src/finance_insight_service/api_server.py
Outdated
Show resolved
Hide resolved
| if set(selected_tasks) == set(full_order): | ||
| quant_task.context = [research_task] | ||
| audit_task.context = [research_task, quant_task] | ||
| report_task.context = [research_task, quant_task, audit_task] |
There was a problem hiding this comment.
Context wiring assumes full sequential order but only checks set equality.
Line 118 uses set(selected_tasks) == set(full_order) which ignores task order. If a caller passes task_names=["report", "quant", "audit", "research"], the set comparison succeeds and context is wired, but tasks execute in the caller-specified (reversed) order. This would cause quant_task.context = [research_task] to reference a task that runs after quant, not before.
This is an edge case (callers typically use default order), but consider either:
- Checking list equality instead:
selected_tasks == full_order - Documenting that custom task order is not supported when context wiring is needed.
🤖 Prompt for AI Agents
In `@samples/finance-insight-agent/src/finance_insight_service/crew.py` around
lines 118 - 121, The context wiring uses a set comparison that ignores order, so
when selected_tasks is a permutation of full_order you may wire contexts
pointing to tasks that will run later; change the guard from set(selected_tasks)
== set(full_order) to an ordered comparison selected_tasks == full_order (or
explicitly check ordering) before assigning contexts to quant_task, audit_task,
and report_task so those context dependencies only get wired when the execution
order matches; alternatively add a clear docstring/comment on the function
explaining custom task ordering is unsupported when context wiring is applied.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 102 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "\n".join(normalized) | ||
|
|
||
|
|
||
| EXEC_TIMEOUT_SECONDS = 600 |
There was a problem hiding this comment.
The EXEC_TIMEOUT_SECONDS is set to 600 seconds (10 minutes), which is very long for a synchronous code execution in an API context. This could cause request timeouts or resource exhaustion. Consider reducing this to 30-60 seconds to prevent long-running executions from blocking the agent workflow.
| EXEC_TIMEOUT_SECONDS = 600 | |
| EXEC_TIMEOUT_SECONDS = 60 |
| # NOTE: Including `type` enables basic introspection. This sandbox is | ||
| # only for trusted, agent-generated code and is not a security | ||
| # boundary for untrusted input. | ||
| "type": type, |
There was a problem hiding this comment.
The code includes type in safe_builtins with a comment acknowledging it enables introspection and is "not a security boundary for untrusted input." However, including type allows access to the __class__ attribute which can be used to escape the sandbox. Even for "trusted" agent-generated code, this is risky as agents can make mistakes or be manipulated. Consider removing type from the allowed builtins or adding additional safeguards.
| # NOTE: Including `type` enables basic introspection. This sandbox is | |
| # only for trusted, agent-generated code and is not a security | |
| # boundary for untrusted input. | |
| "type": type, | |
| # NOTE: This sandbox is only for trusted, agent-generated code and is | |
| # not a security boundary for untrusted input. |
| try { | ||
| localStorage.setItem("financeApiKeys", JSON.stringify(keys)); | ||
| } catch (error) { | ||
| console.warn("[Settings] localStorage unavailable:", error); | ||
| } | ||
|
|
There was a problem hiding this comment.
The Settings page stores API keys in localStorage, which is vulnerable to XSS attacks. The warning at line 257 acknowledges this, but the implementation doesn't enforce secure practices. Consider: 1) Not allowing API keys to be stored in the frontend at all (backend-only), 2) Using secure, httpOnly cookies if keys must be client-side, or 3) Implementing Content Security Policy headers to mitigate XSS risks.
| outputsize = int(payload.get("outputsize", 260) or 260) | ||
| horizon_days = int(payload.get("horizon_days", 30) or 30) | ||
| provided_data = payload.get("provided_data", "") | ||
| if isinstance(provided_data, (dict, list)): | ||
| provided_data = json.dumps(provided_data) | ||
| search_query = _build_search_query(query, tickers, sites) | ||
|
|
||
| # Get current date/time to provide context | ||
| current_date = datetime.now().strftime("%Y-%m-%d") | ||
| current_year = datetime.now().year | ||
| days = int(payload.get("days", 7) or 7) | ||
| max_articles = int(payload.get("max_articles", 8) or 8) |
There was a problem hiding this comment.
Missing input validation for numeric parameters. The code calls int() on user-provided values with fallbacks, but doesn't validate the ranges before using them. A malicious actor could provide negative values or extremely large values that could cause issues. Add explicit range validation (e.g., ensure days > 0, outputsize between reasonable bounds like 10-5000).
| "crewai[tools]>=1.8.0,<1.9.0", | ||
| "crewai-tools>=1.8.0,<1.9.0", |
There was a problem hiding this comment.
The crewAI dependency version is pinned to >=1.8.0,<1.9.0 in pyproject.toml. This is a narrow version range that may cause compatibility issues. If crewAI releases 1.9.0 with breaking changes, this is good. However, ensure this aligns with the features used in the code (like the events API and instrumentation).
| "crewai[tools]>=1.8.0,<1.9.0", | |
| "crewai-tools>=1.8.0,<1.9.0", | |
| "crewai[tools]>=1.8.0,<2.0.0", | |
| "crewai-tools>=1.8.0,<2.0.0", |
| for unsub in unsubscribe_funcs: | ||
| try: | ||
| unsub() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| normalized.append(json.loads(text)) | ||
| continue | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| normalized.append(ast.literal_eval(text)) | ||
| continue | ||
| except (ValueError, SyntaxError): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY . . | ||
|
|
||
| RUN pip install --no-cache-dir uv \ | ||
| && uv sync --frozen | ||
|
|
||
| ENV VIRTUAL_ENV=/app/.venv | ||
| ENV PATH="$VIRTUAL_ENV/bin:$PATH" | ||
|
|
There was a problem hiding this comment.
The container image runs the application as the default root user because no non-privileged USER is defined before the CMD, which is a risky configuration in production. If an attacker exploits the app or a dependency, root inside the container increases the potential impact (e.g., easier container escape, filesystem damage, or lateral movement). Define and switch to a dedicated non-root user (with only the minimal required permissions) before starting finance_insight_service.api_server to reduce privilege and harden the runtime environment.
| && rm -rf /var/lib/apt/lists/* | |
| COPY . . | |
| RUN pip install --no-cache-dir uv \ | |
| && uv sync --frozen | |
| ENV VIRTUAL_ENV=/app/.venv | |
| ENV PATH="$VIRTUAL_ENV/bin:$PATH" | |
| && rm -rf /var/lib/apt/lists/* \ | |
| && useradd --create-home --shell /usr/sbin/nologin appuser | |
| COPY . . | |
| RUN chown -R appuser:appuser /app | |
| RUN pip install --no-cache-dir uv \ | |
| && uv sync --frozen | |
| ENV VIRTUAL_ENV=/app/.venv | |
| ENV PATH="$VIRTUAL_ENV/bin:$PATH" | |
| USER appuser |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@samples/finance-insight-agent/.env.example`:
- Around line 8-14: The .env example uses inline/trailing comments and extra
spaces that some parsers will include in variable values; update the file so
each variable (SERPAPI_API_KEY, TWELVE_DATA_API_KEY, ALPHAVANTAGE_API_KEY) is a
plain KEY=VALUE line with no trailing comments or leading/trailing spaces and
move the explanatory comments to their own lines above each variable (e.g., "#
Get from serpapi.com" on the previous line), leaving the variable lines blank
after the '=' to indicate required values.
In `@samples/finance-insight-agent/src/finance_insight_service/api_server.py`:
- Around line 449-459: The current unsubscription logic is incorrect because
crewai_event_bus.on(event_cls)(handler_wrapper) returns the handler, not an
unsubscribe function, so calling unsub() invokes the handler instead of removing
it; replace the manual subscribe/unsubscribe pattern (unsubscribe_funcs and the
finally loop that calls unsub()) with the context manager
crewai_event_bus.scoped_handlers(): enter a with
crewai_event_bus.scoped_handlers(): block, register handlers via
crewai_event_bus.on(event_cls)(handler_wrapper) inside that block, then call
result = crew.kickoff(inputs=inputs) inside the context so handlers are
automatically removed on exit (remove unsubscribe_funcs, the try/finally, and
the manual unsub loop).
In
`@samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.py`:
- Around line 28-38: The _run method currently calls int(limit) directly which
can raise ValueError if called with a non-numeric string; update _run to safely
coerce/validate limit by wrapping the int conversion in a try/except (catch
ValueError and TypeError) and return an _error_payload (e.g., "limit must be an
integer") on failure, then apply the existing bounding logic limit = max(1,
min(int_limit, 12)); reference the _run function and the limit variable so the
change is applied where int(limit) is used.
In
`@samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py`:
- Around line 121-135: The code currently skips parsing empty JSON containers
because it checks `if data_json:`; change this to an explicit `if data_json is
not None:` so empty objects/arrays (`{}`, `[]`) get passed to
`_parse_json_payload()` and validated; ensure `context["data"]` is set from
`_parse_json_payload(data_json)` in the try/except that already returns the
`CODE_ERROR` JSON on ValueError, and leave the `else` branch to set
`context["data"] = None` only when `data_json` is actually None.
- Around line 136-160: The current exec block in safe_python_exec.py that
installs SIGALRM (references: signal.signal, signal.alarm, _timeout_handler,
EXEC_TIMEOUT_SECONDS, code_to_run, context) silently disables timeouts when run
from non-main threads; replace this unsafe process-wide alarm approach by
executing the user code in a subprocess (preferred) or by explicitly detecting
non-main thread and raising a clear error. Specifically: remove the
signal.signal/ signal.alarm usage around exec(code_to_run, context) and instead
run the code in a separate subprocess (spawn a short-lived Python process that
receives code_to_run, enforces timeout via
subprocess.run(timeout=EXEC_TIMEOUT_SECONDS), captures stdout/stderr into the
same stdout variable, and returns equivalent JSON shapes for CODE_ERROR,
TIMEOUT, and success); if you choose the simpler interim fix, detect
threading.current_thread().name != "MainThread" (or not threading.main_thread())
before installing SIGALRM and raise a clear RuntimeError indicating timeouts are
unsupported in worker threads (do not silently proceed). Ensure returned JSON
keys and traceback handling remain compatible with existing callers.
🧹 Nitpick comments (10)
samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.py (2)
13-16: Add Pydantic constraints forsymbolandlimitto fail fast.This reduces reliance on manual clamping and avoids surprise coercions.
🔧 Suggested update
class CompanyFundamentalsFetchArgs(BaseModel): """Arguments for company fundamentals fetching.""" - symbol: str = Field(..., description="Ticker or symbol to fetch fundamentals for.") - limit: int = Field(1, description="Number of periods to return (latest first).") + symbol: str = Field( + ..., + min_length=1, + description="Ticker or symbol to fetch fundamentals for.", + ) + limit: int = Field( + 1, + ge=1, + le=12, + description="Number of periods to return (latest first).", + )
72-76: Avoid returning raw exception text in the user-facing error payload.Consider returning a generic message and logging details internally to reduce information leakage.
🔒 Suggested update
- except Exception as exc: - return {}, f"{function} request failed: {exc}" + except Exception: + return {}, f"{function} request failed"samples/finance-insight-agent/src/ui/PROJECT_GUIDE.md (1)
44-52: Add language specifiers to fenced code blocks.The code blocks showing URLs should have a language identifier to satisfy markdown linters.
🔧 Proposed fix
-``` +```text http://default.localhost:9080/finance-insightOverride this for local development or other deployments via
src/ui/.env.local:-
+text
NEXT_PUBLIC_API_BASE_URL=http://localhost:5000samples/finance-insight-agent/src/finance_insight_service/tools/price_history_fetch.py (1)
49-49: Samedatetime.utcnow()deprecation applies here.Consider updating to
datetime.now(timezone.utc)for consistency with Python 3.12+ recommendations, as noted in the serpapi_news_search.py review.Also applies to: 111-111, 123-123
samples/finance-insight-agent/src/ui/lib/api.ts (2)
131-136: Consider checking status immediately before the first delay.The polling delay at line 136 runs before the first status check, meaning there's always a 2-second wait after job submission before any status is retrieved. For faster-completing jobs, moving the delay after the status check would improve responsiveness.
🔧 Proposed change
const pollStatus = async (): Promise<ReportResponse> => { while (true) { if (signal?.aborted) { throw new DOMException("Request cancelled", "AbortError"); } - await new Promise(resolve => setTimeout(resolve, 2000)); // Poll every 2 seconds try { const statusResponse = await fetch(`${baseUrl}/chat/async/${jobId}/status`, { // ...existing code... }); // ...existing status handling... if (statusData.status === 'completed' || statusData.status === 'failed' || statusData.status === 'cancelled') { // ...existing completion handling... } + await new Promise(resolve => setTimeout(resolve, 2000)); // Poll every 2 seconds } catch (error) { // ...existing error handling... } } };
58-63: Consider adding runtime validation for parsed config.The
JSON.parseresult is cast toApiConfigwithout validation. If localStorage contains malformed data, this could cause subtle issues.💡 Example validation approach
const parsed = JSON.parse(stored); if (typeof parsed === 'object' && parsed !== null) { return { baseUrl: typeof parsed.baseUrl === 'string' ? parsed.baseUrl : DEFAULT_API_BASE_URL, apiKey: typeof parsed.apiKey === 'string' ? parsed.apiKey : "", }; } return { baseUrl: DEFAULT_API_BASE_URL };samples/finance-insight-agent/src/finance_insight_service/tools/serpapi_news_search.py (1)
75-75: Replacedatetime.utcnow()withdatetime.now(timezone.utc)–datetime.utcnow()is deprecated as of Python 3.12 and returns a naive datetime. This occurs at line 75 and line 119.🔧 Proposed fix
import json import os -from datetime import datetime +from datetime import datetime, timezone from typing import AnyThen replace both usages:
- "fetched_at": datetime.utcnow().isoformat() + "Z", + "fetched_at": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"),samples/finance-insight-agent/src/finance_insight_service/api_server.py (3)
188-191: Minor inconsistency:datetime.now()vsdatetime.now(timezone.utc).Lines 189-190 use
datetime.now()(local time) while the rest of the codebase uses_utc_now()(UTC). This could cause confusion if the server runs in a non-UTC timezone, ascurrent_dateshown to agents would differ from timestamps in traces.♻️ Suggested fix for consistency
# Get current date/time to provide context - current_date = datetime.now().strftime("%Y-%m-%d") - current_year = datetime.now().year + now = _utc_now() + current_date = now.strftime("%Y-%m-%d") + current_year = now.year
288-300: Memory reporting is misleading.
sys.getsizeof(jobs)only returns the size of the dictionary object itself (~200-300 bytes), not the cumulative size of all job data stored within it. This metric will remain nearly constant regardless of how many jobs are stored.♻️ Suggested fix for more accurate memory reporting
+import sys + +def _estimate_job_memory(jobs_dict: dict) -> float: + """Rough estimate of memory used by jobs (in MB).""" + total = sys.getsizeof(jobs_dict) + for job_id, job in jobs_dict.items(): + total += sys.getsizeof(job_id) + total += sys.getsizeof(job) + for k, v in job.items(): + total += sys.getsizeof(k) + sys.getsizeof(v) + if isinstance(v, list): + for item in v: + total += sys.getsizeof(item) + return total / (1024 * 1024)Then use
_estimate_job_memory(jobs)instead ofsys.getsizeof(jobs) / (1024 * 1024).
513-528: Cancellation is best-effort only.The cancel endpoint sets the job status to
CANCELLED, but this doesn't actually interrupt the running crew execution. The job thread checks_is_job_cancelled()at specific points (lines 350, 461, 474), so long-running tool calls or LLM requests will continue until completion.This is acceptable for a sample, but consider documenting this limitation in the API response or comments.
samples/finance-insight-agent/src/finance_insight_service/api_server.py
Outdated
Show resolved
Hide resolved
| def _run(self, symbol: str, limit: int = 1) -> str: | ||
| """Fetch and return fundamentals for a ticker symbol.""" | ||
| symbol = (symbol or "").strip().upper() | ||
| if not symbol: | ||
| return _error_payload("symbol is required") | ||
|
|
||
| api_key = (os.getenv("ALPHAVANTAGE_API_KEY") or "").strip().strip("\"'").strip() | ||
| if not api_key: | ||
| return _error_payload("ALPHAVANTAGE_API_KEY missing", provider="alpha_vantage") | ||
|
|
||
| limit = max(1, min(int(limit), 12)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.pyRepository: wso2/agent-manager
Length of output: 5383
🏁 Script executed:
rg -n "CompanyFundamentalsFetchTool|_run.*limit" samples/finance-insight-agent --type py -A 2 -B 2Repository: wso2/agent-manager
Length of output: 2457
🏁 Script executed:
fd -e py -path "*/test*" samples/finance-insight-agent | head -20Repository: wso2/agent-manager
Length of output: 230
🏁 Script executed:
find samples/finance-insight-agent -name "*test*.py" -o -name "test_*.py"Repository: wso2/agent-manager
Length of output: 44
🏁 Script executed:
rg -n "def _run|@" samples/finance-insight-agent/src/finance_insight_service/tools --type py -B 2 | head -50Repository: wso2/agent-manager
Length of output: 1623
🏁 Script executed:
cat -n samples/finance-insight-agent/src/finance_insight_service/tools/price_history_fetch.py | sed -n '29,60p'Repository: wso2/agent-manager
Length of output: 1402
🏁 Script executed:
cat -n samples/finance-insight-agent/src/finance_insight_service/tools/price_history_fetch.py | sed -n '1,30p'Repository: wso2/agent-manager
Length of output: 1313
Guard int(limit) to prevent crashes if schema validation is bypassed.
The limit parameter has a Pydantic schema enforcing int type, which provides protection during normal CrewAI invocation. However, if _run() is called directly with a non-numeric string, int(limit) will raise ValueError. This is inconsistent with how symbol and api_key are handled elsewhere in the function.
🛡️ Suggested fix
- limit = max(1, min(int(limit), 12))
+ try:
+ limit = int(limit)
+ except (TypeError, ValueError):
+ return _error_payload("limit must be an integer between 1 and 12")
+ limit = max(1, min(limit, 12))📝 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.
| def _run(self, symbol: str, limit: int = 1) -> str: | |
| """Fetch and return fundamentals for a ticker symbol.""" | |
| symbol = (symbol or "").strip().upper() | |
| if not symbol: | |
| return _error_payload("symbol is required") | |
| api_key = (os.getenv("ALPHAVANTAGE_API_KEY") or "").strip().strip("\"'").strip() | |
| if not api_key: | |
| return _error_payload("ALPHAVANTAGE_API_KEY missing", provider="alpha_vantage") | |
| limit = max(1, min(int(limit), 12)) | |
| def _run(self, symbol: str, limit: int = 1) -> str: | |
| """Fetch and return fundamentals for a ticker symbol.""" | |
| symbol = (symbol or "").strip().upper() | |
| if not symbol: | |
| return _error_payload("symbol is required") | |
| api_key = (os.getenv("ALPHAVANTAGE_API_KEY") or "").strip().strip("\"'").strip() | |
| if not api_key: | |
| return _error_payload("ALPHAVANTAGE_API_KEY missing", provider="alpha_vantage") | |
| try: | |
| limit = int(limit) | |
| except (TypeError, ValueError): | |
| return _error_payload("limit must be an integer between 1 and 12") | |
| limit = max(1, min(limit, 12)) |
🤖 Prompt for AI Agents
In
`@samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.py`
around lines 28 - 38, The _run method currently calls int(limit) directly which
can raise ValueError if called with a non-numeric string; update _run to safely
coerce/validate limit by wrapping the int conversion in a try/except (catch
ValueError and TypeError) and return an _error_payload (e.g., "limit must be an
integer") on failure, then apply the existing bounding logic limit = max(1,
min(int_limit, 12)); reference the _run function and the limit variable so the
change is applied where int(limit) is used.
samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py
Outdated
Show resolved
Hide resolved
samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@samples/finance-insight-agent/.env.example`:
- Around line 4-6: The .env.example currently uses a realistic-looking API key
prefix ("OPENAI_API_KEY=sk-...") which can trigger secret scanners; update the
placeholder for the OPENAI_API_KEY entry to a neutral value such as
"OPENAI_API_KEY=" or "OPENAI_API_KEY=YOUR_OPENAI_API_KEY" (i.e., remove the
"sk-" prefix and any realistic characters) so the template no longer resembles a
real secret.
🧹 Nitpick comments (5)
samples/finance-insight-agent/src/finance_insight_service/tools/safe_python_exec.py (2)
66-67: Unused function definition.
_deny_exitis defined inside_run()but never used. The actual_deny_exitthat's used is defined in_EXEC_RUNNER(line 276). This dead code can be removed.🧹 Proposed fix
- def _deny_exit(*_args, **_kwargs): - raise RuntimeError("exit is not allowed; return limitations instead") - data_payload = None
209-242: Indentation normalization is heuristic and may alter valid code.The
_normalize_indentationfunction uses simple heuristics that can incorrectly modify valid Python code. For example, dedentingelif/elseby 4 spaces fromprev_indent(line 229) doesn't account for the actual indentation level of the correspondingifstatement.Since this is a best-effort tool for agent-generated code, consider adding a docstring note about its limitations or wrapping usage in a try/except to fall back to the original code if the normalized version fails to parse.
samples/finance-insight-agent/src/finance_insight_service/tools/company_fundamentals_fetch.py (1)
57-57:datetime.utcnow()is deprecated; use timezone-aware alternative.
datetime.utcnow()is deprecated since Python 3.12 (PEP 587). For consistency withapi_server.pywhich usesdatetime.now(timezone.utc), update this to return timezone-aware UTC times.♻️ Proposed fix
Update the import at the top of the file:
-from datetime import datetime +from datetime import datetime, timezoneThen update line 57:
- "fetched_at": datetime.utcnow().isoformat() + "Z", + "fetched_at": datetime.now(timezone.utc).isoformat(),Apply the same fix to line 113 in
_error_payload().samples/finance-insight-agent/src/finance_insight_service/api_server.py (2)
307-307:sys.getsizeof(jobs)doesn't measure actual job data memory.
sys.getsizeof(jobs)returns only the size of the dictionary object itself (a few hundred bytes), not the size of its contents. For meaningful memory monitoring, consider usingsys.getsizeofrecursively or a library likepympler, or simply reportlen(jobs)which is already included.🔧 Simpler alternative
Since
len(jobs)is already reported astotal, consider removing the misleadingmemory_mbfield or replacing it with a rough estimate:- "memory_mb": sys.getsizeof(jobs) / (1024 * 1024), + "memory_mb": len(jobs) * 0.01, # Rough estimate: ~10KB per jobOr remove it entirely since it's not accurate.
459-460: Emptyfinallyblock is unnecessary.The
finally: passblock serves no purpose and can be removed.🧹 Proposed fix
result = crew.kickoff(inputs=inputs) - finally: - pass if _is_job_cancelled(job_id):
| # Required: OpenAI API for AI agents (GPT-4, embeddings) | ||
| OPENAI_API_KEY=sk-... | ||
|
|
There was a problem hiding this comment.
Avoid real-looking key prefixes in templates.
OPENAI_API_KEY=sk-... can trip secret scanners and encourages copying a real-looking key. Use a neutral placeholder (e.g., OPENAI_API_KEY= or OPENAI_API_KEY=YOUR_OPENAI_API_KEY).
🔧 Suggested change
-OPENAI_API_KEY=sk-...
+OPENAI_API_KEY=📝 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.
| # Required: OpenAI API for AI agents (GPT-4, embeddings) | |
| OPENAI_API_KEY=sk-... | |
| # Required: OpenAI API for AI agents (GPT-4, embeddings) | |
| OPENAI_API_KEY= | |
🤖 Prompt for AI Agents
In `@samples/finance-insight-agent/.env.example` around lines 4 - 6, The
.env.example currently uses a realistic-looking API key prefix
("OPENAI_API_KEY=sk-...") which can trigger secret scanners; update the
placeholder for the OPENAI_API_KEY entry to a neutral value such as
"OPENAI_API_KEY=" or "OPENAI_API_KEY=YOUR_OPENAI_API_KEY" (i.e., remove the
"sk-" prefix and any realistic characters) so the template no longer resembles a
real secret.
Adding The agent sample- Finance-Insight-Agent.
Summary by CodeRabbit
New Features
Documentation
Chores