Polish repo tooling, docs, and quality automation - #10
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughComprehensive refactor transitioning documentation to static GitHub Pages with client-side GitHub issue draft feedback, consolidating Node dependencies into a pnpm root workspace, introducing semantic-release versioning, and adding Sonar scanning with local/cloud targets and scripted infrastructure for releases, QA, and Makefile-based command aliases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review spans heterogeneous changes across documentation, infrastructure, Python logic refactoring, JavaScript API routing, and deployment scripting. While individual cohorts are coherent, their interactions (e.g., CLI package-manager resolution affecting TUI startup, docs feedback drafts affecting web GUI, Sonar targets affecting CI scripts) require cross-cutting understanding. The substantial removal of server-side feedback infrastructure and migration to static export is a significant architectural shift requiring careful validation. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ogiboy
marked this pull request as ready for review
April 27, 2026 19:33
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @ogiboy. The following files were modified: * `agentic_trader/agents/fundamental.py` * `agentic_trader/cli.py` * `agentic_trader/market/features.py` * `docs/app/(landing)/layout.tsx` * `docs/app/[lang]/layout.tsx` * `docs/components/docs/doc-page-shell.tsx` * `docs/components/feedback/client.tsx` * `docs/components/feedback/feedback-result.tsx` * `docs/components/ui/badge.tsx` * `scripts/qa/run_sonar_scan.sh` * `scripts/qa/smoke_qa.py` * `scripts/release/version_plan.py` * `scripts/secrets/keychain-get.sh` * `tests/test_backtest.py` * `tests/test_features.py` * `webgui/src/app/api/runtime/route.ts` These files were kept as they were: * `tests/test_fundamental_helpers.py` * `webgui/src/app/api/chat/route.ts` * `webgui/src/app/api/instruct/route.ts` These file types are not supported: * `.ai/agents/implementer.md` * `.ai/current-state.md` * `.ai/decisions.md` * `.ai/qa/qa-checklist.md` * `.ai/qa/qa-runbook.md` * `.ai/qa/qa-scenarios.md` * `.ai/qa/qa-smoke-script.md` * `.ai/tasks.md` * `.env.example` * `.github/pull_request_template.md` * `.gitignore` * `CHANGELOG.md` * `Makefile` * `README.md` * `docs/.env.example` * `docs/AGENTS.md` * `docs/content/docs/en/contributing.mdx` * `docs/content/docs/en/frontend-system.mdx` * `docs/content/docs/en/getting-started.mdx` * `docs/content/docs/en/memory-and-review.mdx` * `docs/content/docs/en/qa-and-debugging.mdx` * `docs/content/docs/tr/contributing.mdx` * `docs/content/docs/tr/frontend-system.mdx` * `docs/content/docs/tr/getting-started.mdx` * `docs/content/docs/tr/memory-and-review.mdx` * `docs/content/docs/tr/qa-and-debugging.mdx` * `docs/public/site.webmanifest` * `sonar-project.properties` * `webgui/.env.example` * `webgui/AGENTS.md`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webgui/src/app/api/instruct/route.ts (1)
43-55:⚠️ Potential issue | 🟡 MinorSame non-string
message→ 500 hazard aschat/route.ts.
body.message?.trim()at Line 55 throws ifmessageis, say, a number or object, and the error then surfaces as a 500 from the outer catch instead of a 400. Tightening the field check totypeof body.message !== 'string'(and similarly guardingbody.apply) would keep malformed-client responses on the 4xx path.🛡️ Proposed fix
- let body: { message?: string; apply?: boolean }; + let body: { message?: unknown; apply?: unknown }; try { const parsed: unknown = await request.json(); - if (typeof parsed !== 'object' || parsed === null) { + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { return Response.json({ error: 'invalid json' }, { status: 400 }); } - body = parsed as { message?: string; apply?: boolean }; + body = parsed as { message?: unknown; apply?: unknown }; } catch { return Response.json({ error: 'invalid json' }, { status: 400 }); } try { - if (!body.message?.trim()) { + if (typeof body.message !== 'string' || !body.message.trim()) { return Response.json( { error: 'missing instruction message' }, { status: 400 }, ); } const apply = body.apply === true; const result = await runInstruction(body.message, apply);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webgui/src/app/api/instruct/route.ts` around lines 43 - 55, The handler currently uses body.message?.trim() which will throw for non-string values and escalate to a 500; update the request validation in route.ts to explicitly check types before calling trim: confirm typeof body.message === 'string' and that body.message.trim() is non-empty, and validate body.apply with typeof body.apply === 'boolean' (or undefined) before using it; return a 400 Response.json({ error: 'invalid request' }, { status: 400 }) for bad types so malformed client input stays on the 4xx path rather than causing an exception in the outer try.scripts/qa/smoke_qa.py (1)
421-429:⚠️ Potential issue | 🟡 MinorRedact exception details in
CheckResultmetadata as well.The artifact content is redacted, but
detailscurrently uses rawexc, which is printed and written tosmoke-summary.json.🔒 Suggested fix
except Exception as exc: exception_text = _redact_sensitive_text(str(exc), sensitive_values) _write_artifact( artifact, f"$ {display_command}\n\nEXCEPTION:\n{exception_text}\n" ) return CheckResult( name=name, passed=False, - details=f"exception={exc}", + details=f"exception={exception_text}", artifact=str(artifact), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/qa/smoke_qa.py` around lines 421 - 429, The CheckResult currently embeds the raw exception object in the details field which leaks sensitive data; replace the unredacted exc with the already-redacted exception_text (produced by _redact_sensitive_text) when constructing the CheckResult (the code path that calls CheckResult(name=name, passed=False, details=..., artifact=...)), ensuring details uses exception_text (string) rather than exc; reference symbols: _redact_sensitive_text, exception_text, _write_artifact, and CheckResult so you update the details assignment to include the redacted text.
🧹 Nitpick comments (22)
.github/pull_request_template.md (1)
9-12: Optional: require command + result in## Testing.Consider replacing the freeform
-with a short prompt format (e.g.,command,result,evidence path) so validations are reproducible and auditable.Proposed tweak
## Testing -- +- Command(s): +- Result(s): +- Evidence (log/artifact path):Based on learnings: Run the smallest useful validation for this change and keep workflow assumptions explicit in
.ai/docs updates.Also applies to: 22-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/pull_request_template.md around lines 9 - 12, Update the "## Testing" section in the PR template by replacing the placeholder "-" with a short, structured prompt (for example: `command`, `expected result`, `evidence path`) so reviewers can reproduce validations; change the content under the "## Testing" header (referenced as the "## Testing" block) to require those three fields and apply the same change to the other occurrences noted (lines 22-23 equivalent sections) to keep testing instructions consistent and auditable..gitignore (1)
52-52: Broadenout/ignore pattern to avoid edge-case misses.At Line 52,
*/out/can miss some layouts (notably root-levelout/). Prefer a recursive rule.Suggested update
-*/out/ +**/out/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 52, Replace the narrow ignore pattern "*/out/" in .gitignore with a recursive rule so root-level and nested out/ directories are ignored; locate the current "*/out/" entry and change it to a recursive pattern such as "**/out/" (or simply "out/") to ensure all out/ directories are covered.agentic_trader/market/features.py (1)
95-97: Innercastin_as_floatis redundant.
typing.castis a no-op at runtime, sofloat(cast(float, value))is equivalent tofloat(value). Since the wrapper exists purely to satisfy the type checker overpd.Series/scalar element access, you can drop the inner cast.♻️ Proposed simplification
-def _as_float(value: object) -> float: - return float(cast(float, value)) +def _as_float(value: object) -> float: + return float(value) # type: ignore[arg-type]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agentic_trader/market/features.py` around lines 95 - 97, The helper _as_float contains a redundant typing.cast call; replace the implementation of _as_float to remove the inner cast and simply convert the input to float (i.e. use float(value)) so the runtime is not doing a no-op cast — update the function named _as_float accordingly so the type-checker still sees it as a float conversion but the code uses float(value) directly.tests/test_backtest.py (1)
41-45: Consider consolidating withtests/test_features.py.The same ISO-formatting pattern is open-coded in
tests/test_features.py(line 86–87). If this idiom is needed in a third place, consider moving_index_isoto a sharedtests/conftest.pyortests/_helpers.pymodule and importing it there to keep the timestamp-assertion approach uniform across the suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_backtest.py` around lines 41 - 45, Duplicate ISO-formatting of index values is present in _index_iso (tests/test_backtest.py) and in tests/test_features.py; extract this helper into a shared test helper (e.g., add a function iso_index or _index_iso in tests/conftest.py or tests/_helpers.py) and update both tests to import and call that shared helper instead of duplicating the code, ensuring the helper accepts a DataFrame and position and returns str(value.isoformat()) so existing assertions keep working.agentic_trader/agents/fundamental.py (2)
279-308: Good helper extraction; consider adding short docstrings.
_fallback_risk_flags,_fallback_strengths, and_has_provider_gapcleanly factor out reusable logic from_fallback_fundamental. The other helpers in this module all carry docstrings — adding one-liners here would keep the file consistent and aid future readers, especially clarifying that_has_provider_gaptreats a missing context/decision_features as a gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agentic_trader/agents/fundamental.py` around lines 279 - 308, Add concise one-line docstrings to _fallback_risk_flags, _fallback_strengths, and _has_provider_gap describing their purpose and return values; for _has_provider_gap explicitly note that a missing context or missing context.decision_features is treated as a provider gap (returns True). Keep each docstring short and placed immediately under the def line for the corresponding functions (_fallback_risk_flags, _fallback_strengths, _has_provider_gap).
420-430: ReusePROVIDER_GAP_FLAGSinstead of duplicating the set here.The hard-coded
missing_flagsset on lines 425-429 mirrorsPROVIDER_GAP_FLAGSdefined at the top of the module. Reusing the constant prevents the two from drifting apart if a new provider-gap flag is added to one but not the other.♻️ Proposed refactor
def _has_structured_fundamental_evidence(context: AgentContext | None) -> bool: """Return whether the context contains real provider-backed fundamentals.""" if context is None or context.decision_features is None: return False flags = set(context.decision_features.fundamental.quality_flags) - missing_flags = { - "fundamental_provider_missing", - "fundamental_fetch_not_implemented", - "fundamental_provider_not_configured", - } - return not bool(flags.intersection(missing_flags)) + return flags.isdisjoint(PROVIDER_GAP_FLAGS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agentic_trader/agents/fundamental.py` around lines 420 - 430, The function _has_structured_fundamental_evidence duplicates the set of provider-gap flags; replace the local missing_flags set with the module-level constant PROVIDER_GAP_FLAGS to avoid drift. Update _has_structured_fundamental_evidence to use PROVIDER_GAP_FLAGS (keeping the existing behavior of checking intersection with flags from context.decision_features.fundamental.quality_flags) and remove the hard-coded missing_flags variable so the function references the single source of truth.webgui/src/app/api/chat/route.ts (1)
41-50: Consider extracting the JSON-body parser into a shared helper.The exact same
try { request.json() → typeof object && !== null → cast } catch { 400 invalid json }block now appears inchat/route.ts,instruct/route.ts,runtime/route.ts(and per the AI summary,dashboard/route.ts). A small helper inwebgui/src/lib/would deduplicate this and let you tighten validation (e.g.,Array.isArrayrejection, schema check) in one place.♻️ Sketch
// webgui/src/lib/http/parse-json-body.ts export async function parseJsonObjectBody( request: Request, ): Promise<{ ok: true; body: Record<string, unknown> } | { ok: false; response: Response }> { try { const parsed: unknown = await request.json(); if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { return { ok: false, response: Response.json({ error: 'invalid json' }, { status: 400 }) }; } return { ok: true, body: parsed as Record<string, unknown> }; } catch { return { ok: false, response: Response.json({ error: 'invalid json' }, { status: 400 }) }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webgui/src/app/api/chat/route.ts` around lines 41 - 50, Extract the repeated JSON parsing logic into a shared helper named parseJsonObjectBody that takes a Request and returns either { ok: true; body: Record<string, unknown> } or { ok: false; response: Response }; replace the try/ catch + typeof/null check (the block that calls request.json(), validates typeof object && !== null and returns Response.json({ error: 'invalid json' }, { status: 400 })) in chat route handler with a call to this helper and early-return the helper.response when ok is false, then cast the returned body to your existing local shape (e.g., the body variable used in chat/route.ts) and apply the same replacement in instruct/route.ts, runtime/route.ts and dashboard/route.ts so all request.json() parsing is centralized and can be tightened (e.g., reject Array.isArray) in one place.scripts/sonarqube/start-local.sh (1)
7-12: Consider validating thedocker composeplugin and the compose file path.
command -v dockeronly confirms the Docker CLI; on systems with only the legacydocker-composebinary,docker compose ... up -dwill fail with a less obvious error. Also, an invalidSONARQUBE_COMPOSE_FILEoverride produces a noisy compose error rather than a clear hint.♻️ Proposed hardening
if ! command -v docker >/dev/null 2>&1; then echo "Docker is required to start local SonarQube." >&2 exit 1 fi + +if ! docker compose version >/dev/null 2>&1; then + echo "Docker Compose v2 plugin is required (docker compose ...)." >&2 + exit 1 +fi + +if [[ ! -f "${COMPOSE_FILE}" ]]; then + echo "Compose file not found: ${COMPOSE_FILE}" >&2 + exit 1 +fi docker compose -f "${COMPOSE_FILE}" up -d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sonarqube/start-local.sh` around lines 7 - 12, Check that the script verifies both the Docker CLI and the Docker Compose capability and that the compose file exists: detect whether "docker compose" is supported (fallback to "docker-compose" if present) before running the compose command, and validate the COMPOSE_FILE (and any override via SONARQUBE_COMPOSE_FILE) path is present and readable; if checks fail, print clear error messages and exit non‑zero instead of invoking docker compose with an invalid configuration. Ensure you update the script around the existing docker check and the docker compose invocation (references: COMPOSE_FILE, SONARQUBE_COMPOSE_FILE, and the "docker compose" / "docker-compose" commands).docs/app/(landing)/layout.tsx (1)
1-3: Quote-style nit (skip if Prettier handles it).Mixed quote styles (
'next/font/google'vs the rest of the repo's preferences) — only worth fixing if the repo's Prettier/ESLint config disagrees. Static analysis didn't flag it, so feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/app/`(landing)/layout.tsx around lines 1 - 3, Summary: Normalize quote style in layout.tsx to match the repo's preferred style. Update the import/export statements in the file so all string literals use the repo-preferred quotes (e.g., change 'next/font/google', '../globals.css', and '@/lib/site-metadata' to the consistent quote style used across the codebase) by editing the top-level import of JetBrains_Mono and the imports/exports so they use the same quote character; ensure references to JetBrains_Mono and the exported docsMetadata as metadata remain unchanged.scripts/sonarqube/status-local.sh (1)
7-9: Redundant--filter name=ondocker ps.
docker ps --filter "name=…"matches as a substring, soname=sonarqubealready matchessonarqube-db. Using two--filter name=predicates also OR's them rather than narrowing the result, so the second filter is a no-op. Either drop it or anchor the regex if you want strict matches.♻️ Suggested simplification
- docker ps --filter "name=sonarqube" --filter "name=sonarqube-db" \ - --format "table {{.Names}}\t{{.Image}}\t{{.Status}}\t{{.Ports}}" + docker ps --filter "name=sonarqube" \ + --format "table {{.Names}}\t{{.Image}}\t{{.Status}}\t{{.Ports}}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sonarqube/status-local.sh` around lines 7 - 9, The docker ps command uses two name filters (--filter "name=sonarqube" and --filter "name=sonarqube-db") which is redundant because name= matches substrings and multiple name filters are ORed; fix by either removing the unnecessary --filter "name=sonarqube" or --filter "name=sonarqube-db" to list only the desired container, or make the match strict by anchoring the regex (e.g., --filter "name=^sonarqube$" and/or --filter "name=^sonarqube-db$") so each filter matches exactly the intended container names.docs/components/feedback/copy.ts (1)
13-17: Property namesopenDiscussionandsuccessForwardedare now stale.The user-visible strings have moved from "GitHub Discussion / forwarded" wording to "GitHub issue / prepared," but the type keys still read
openDiscussionandsuccessForwarded. New contributors readingfeedback-result.tsxwill find aforwarding === "prepared"branch consuming a property literally named "forwarded," which is confusing.Consider renaming for consistency with the new flow:
export type FeedbackCopy = { ... - successForwarded: string; + successIssuePrepared: string; ... - openDiscussion: string; + openIssue: string; genericError: string; };…and update the two consumers (
feedback-result.tsx, plus any test fixtures). Pure rename, no behavior change.Also applies to: 34-41, 56-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/components/feedback/copy.ts` around lines 13 - 17, Rename the stale keys in the feedback copy type and all consumers: change successForwarded -> successPrepared and openDiscussion -> openIssue in docs/components/feedback/copy.ts, then update every usage that reads those properties (notably the forwarding === "prepared" branch in feedback-result.tsx and any test fixtures) to use successPrepared and openIssue; ensure TypeScript types and exported object keys match and run tests to pick up any remaining references.scripts/secrets/install-sonarqube-mcp-wrapper.sh (1)
11-28: Heredoc expands${CANONICAL_SCRIPT}/${FALLBACK_SCRIPT}unquoted into the wrapper.Because
<<EOFis unquoted, the canonical and fallback paths are interpolated at install time as raw shell tokens. IfREPO_ROOTorHOMEever contains$, backticks, or unbalanced quotes, the generated wrapper becomes syntactically broken or will re-expand at runtime. In practice users don't have such paths, so this is a defensive nit only.If you want to harden it, switch to a quoted heredoc and
sed-substitute the two paths in afterward, e.g.:Optional hardening
-cat >"${INSTALL_PATH}" <<EOF -#!/usr/bin/env bash -set -Eeuo pipefail - -SCRIPT="\${AGENTIC_TRADER_SONAR_MCP_SCRIPT:-${CANONICAL_SCRIPT}}" -FALLBACK_SCRIPT="${FALLBACK_SCRIPT}" -... -EOF +cat >"${INSTALL_PATH}" <<'EOF' +#!/usr/bin/env bash +set -Eeuo pipefail + +SCRIPT="${AGENTIC_TRADER_SONAR_MCP_SCRIPT:-__CANONICAL__}" +FALLBACK_SCRIPT="__FALLBACK__" + +if [[ ! -x "${SCRIPT}" && -x "${FALLBACK_SCRIPT}" ]]; then + SCRIPT="${FALLBACK_SCRIPT}" +fi + +if [[ ! -x "${SCRIPT}" ]]; then + echo "SonarQube MCP launcher not found. Set AGENTIC_TRADER_SONAR_MCP_SCRIPT or reinstall from the repo." >&2 + exit 1 +fi + +exec "${SCRIPT}" "$@" +EOF +sed -i.bak \ + -e "s|__CANONICAL__|${CANONICAL_SCRIPT}|" \ + -e "s|__FALLBACK__|${FALLBACK_SCRIPT}|" \ + "${INSTALL_PATH}" && rm -f "${INSTALL_PATH}.bak"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/secrets/install-sonarqube-mcp-wrapper.sh` around lines 11 - 28, The heredoc used to write the wrapper (cat >"${INSTALL_PATH}" <<EOF) allows shell expansion of ${CANONICAL_SCRIPT} and ${FALLBACK_SCRIPT} at install time, which can break the generated script if those values contain special characters; change the write so the heredoc is quoted (e.g. <<'EOF') to prevent interpolation, then explicitly inject the desired values afterward (or perform a safe sed substitution) into the marker variables AGENTIC_TRADER_SONAR_MCP_SCRIPT/CANONICAL_SCRIPT/FALLBACK_SCRIPT within the file; locate the write block around INSTALL_PATH and update the heredoc quoting and post-write substitution logic to safely embed those paths.scripts/release/preview_version_plan.sh (2)
13-13: Use[[ ... ]]for the conditional test.Per the SonarCloud finding,
[[is safer (no word splitting / glob expansion) and more idiomatic in bash.-if [ "$status" -ne 0 ]; then +if [[ "$status" -ne 0 ]]; then tag="" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/preview_version_plan.sh` at line 13, Replace the POSIX test bracket in the release preview conditional with Bash's safer conditional syntax: change the conditional that currently reads `if [ "$status" -ne 0 ]; then` to use `[[ ... ]]` so it becomes `if [[ "$status" -ne 0 ]]; then`, ensuring no word-splitting or glob expansion issues when evaluating the `status` variable in the script `preview_version_plan.sh`.
4-9: Predictable/tmplog path is a minor hardening nit.
2>/tmp/semantic-release-preview.logis a fixed path; on shared hosts this is a symlink-race / collision footgun. Considermktempif you care to harden it:-set +e -tag="$( - poetry run semantic-release --noop version --print-tag 2>/tmp/semantic-release-preview.log \ +set +e +log="$(mktemp -t semantic-release-preview.XXXXXX.log)" +tag="$( + poetry run semantic-release --noop version --print-tag 2>"${log}" \ | grep -E '^v[0-9]+(\.[0-9]+){2}([-.+][0-9A-Za-z.-]+)?$' \ | tail -n 1 )"Also, since
set -euo pipefailenablespipefail, a failingsemantic-release(or agrepwith no matches) propagates to$statuseven though you toggleset +e. The current fallback (tag="") handles that fine — just calling out the behavior so you don't get surprised whengrepexits 1 on empty matches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/preview_version_plan.sh` around lines 4 - 9, Replace the fixed stderr path /tmp/semantic-release-preview.log with a secure temporary file created via mktemp, redirect semantic-release's stderr to that temp file, and ensure the temp file is removed after use; update the pipeline around the semantic-release invocation (the subshell assigning tag from the poetry run semantic-release ... | grep ... | tail -n 1 sequence) to use the temp file and preserve the existing fallback behavior for empty grep matches (so tag stays "" if no match), and consider keeping the set +e around that subshell or explicitly swallowing non-zero exit from grep (e.g., handle grep returning 1) so pipefail doesn't unexpectedly abort the script.docs/lib/site-metadata.ts (1)
3-5: Trailing-slash hygiene onNEXT_PUBLIC_BASE_PATH.If anyone sets
NEXT_PUBLIC_BASE_PATH=/agentic-trader/(with trailing slash) — which is a common copy/paste —assetPath('/favicon.ico')produces/agentic-trader//favicon.ico. Most browsers tolerate it, but the manifest URL inside<link rel="manifest">and downstream tooling may not.-export const basePath = process.env.NEXT_PUBLIC_BASE_PATH ?? ''; - -const assetPath = (path: `/${string}`) => `${basePath}${path}`; +const rawBasePath = process.env.NEXT_PUBLIC_BASE_PATH ?? ''; +export const basePath = rawBasePath.replace(/\/+$/, ''); + +const assetPath = (path: `/${string}`) => `${basePath}${path}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/lib/site-metadata.ts` around lines 3 - 5, The basePath value read from NEXT_PUBLIC_BASE_PATH can contain a trailing slash which causes double-slashes when assetPath concatenates paths; normalize basePath in docs/lib/site-metadata.ts by trimming any trailing slashes and ensuring either an empty string or a single leading slash (e.g., strip /+ from the end and guarantee a leading slash only when non-empty) before using it in assetPath so assetPath('/favicon.ico') never produces a double-slash; update the basePath initialization (symbol: basePath) so assetPath (symbol: assetPath) concatenation is safe regardless of how NEXT_PUBLIC_BASE_PATH is set.scripts/check-python.sh (2)
19-20: Hardcoded/opt/anaconda3/bin/pyrightis developer-machine specific.This fallback only resolves on macOS Anaconda installs at the default prefix. On Linux/WSL/Homebrew Anaconda or Miniconda, the path differs (
/opt/miniconda3/...,~/anaconda3/..., etc.), so the script will silently fall through to the error branch. Either drop this fallback (thecommand -v pyrightarm already covers PATH installs) or make it discoverable, e.g.:-elif [ -x /opt/anaconda3/bin/pyright ]; then - /opt/anaconda3/bin/pyright --pythonpath "${PYTHON_EXEC}" ${PYRIGHT_TARGETS} +elif [ -n "${CONDA_PREFIX:-}" ] && [ -x "${CONDA_PREFIX}/bin/pyright" ]; then + "${CONDA_PREFIX}/bin/pyright" --pythonpath "${PYTHON_EXEC}" ${PYRIGHT_TARGETS}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-python.sh` around lines 19 - 20, The hardcoded fallback invoking /opt/anaconda3/bin/pyright is developer-machine specific and should be removed or made discoverable; change the elif branch in scripts/check-python.sh so it either drops this hardcoded path (relying on the existing command -v pyright path check) or searches common conda/miniconda prefixes (e.g., $HOME/anaconda3, $HOME/miniconda3, /opt/miniconda3) for a pyright binary and then invoke it with the existing arguments (use the same ${PYRIGHT_TARGETS} and --pythonpath "${PYTHON_EXEC}" when found); ensure you still fall back to the current error branch if no pyright is discovered.
16-20: Shellcheck SC2086 is intentional here — silence with a directive.
${PYRIGHT_TARGETS}is meant to word-split into three argv entries. To suppress the noise (and signal intent), add a pragma:+# shellcheck disable=SC2086 # PYRIGHT_TARGETS is intentionally word-split if poetry run sh -c 'command -v pyright >/dev/null 2>&1'; then poetry run pyright ${PYRIGHT_TARGETS}Alternatively, switch to a positional array — but that requires a
bashshebang. For this POSIXshscript, the directive is the cleanest path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-python.sh` around lines 16 - 20, The shellcheck warning SC2086 about unquoted ${PYRIGHT_TARGETS} is intentional because it must split into multiple argv entries; suppress the warning by adding a shellcheck pragma (e.g. a comment "# shellcheck disable=SC2086") immediately above the pyright invocations that use ${PYRIGHT_TARGETS} (the three branches calling "poetry run pyright ${PYRIGHT_TARGETS}", "pyright --pythonpath \"${PYTHON_EXEC}\" ${PYRIGHT_TARGETS}", and "/opt/anaconda3/bin/pyright --pythonpath \"${PYTHON_EXEC}\" ${PYRIGHT_TARGETS}"); keep the rest of the script POSIX-compatible and do not change quoting for ${PYRIGHT_TARGETS}.scripts/secrets/run-sonarqube-mcp.sh (1)
9-9: Fail loudly when the Keychain account cannot be resolved.When neither
SONARQUBE_KEYCHAIN_ACCOUNTnorUSERis set (e.g., minimal CI shells,sudo -Econtexts), this falls back to an empty string and callskeychain-get.sh "${service}" "". Depending onkeychain-get.sh's argument validation, the failure mode can be cryptic. Consider failing fast with a clear message when the account ends up empty.🔧 Proposed fix
SONARQUBE_KEYCHAIN_ACCOUNT="${SONARQUBE_KEYCHAIN_ACCOUNT:-${USER:-}}" +if [[ -z "${SONARQUBE_KEYCHAIN_ACCOUNT}" ]]; then + echo "run-sonarqube-mcp: SONARQUBE_KEYCHAIN_ACCOUNT or USER must be set" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/secrets/run-sonarqube-mcp.sh` at line 9, The SONARQUBE_KEYCHAIN_ACCOUNT assignment may produce an empty value when neither SONARQUBE_KEYCHAIN_ACCOUNT nor USER is set, causing downstream calls like keychain-get.sh "${service}" "" to fail cryptically; after the existing assignment to SONARQUBE_KEYCHAIN_ACCOUNT in run-sonarqube-mcp.sh, add an explicit check that the variable is non-empty and, if empty, print a clear error mentioning SONARQUBE_KEYCHAIN_ACCOUNT and exit non‑zero (so callers see a fast, descriptive failure instead of passing an empty account to keychain-get.sh).Makefile (1)
1-82: Consider adding a defaulthelptarget.With no default target,
make(no args) currently runssetup(the first target). That can be surprising for a thin alias Makefile. A smallhelptarget listing the available aliases — or makinghelpthe default — improves discoverability without changing behavior.♻️ Optional addition
-.PHONY: setup check check-python check-node build qa qa-quality qa-sonar version-plan release-preview sonar sonar-local sonar-cloud sonar-py sonar-js sonar-start sonar-stop sonar-status sonar-secret-check sonarcloud-secret-check sonar-mcp-dry-run sonar-mcp-status sonar-mcp-install-wrapper webgui docs tui clean +.PHONY: help setup check check-python check-node build qa qa-quality qa-sonar version-plan release-preview sonar sonar-local sonar-cloud sonar-py sonar-js sonar-start sonar-stop sonar-status sonar-secret-check sonarcloud-secret-check sonar-mcp-dry-run sonar-mcp-status sonar-mcp-install-wrapper webgui docs tui clean + +help: + `@awk` 'BEGIN{FS=":"} /^[a-zA-Z_-]+:/ && !/^\.PHONY/ {print " make " $$1}' $(MAKEFILE_LIST)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 1 - 82, Add a discoverable default help target to the Makefile and make it the default goal: create a "help" target (and add "help" to the .PHONY list) that prints a brief list of the existing alias targets (e.g., setup, check, build, qa, sonar, webgui, docs, tui, clean), then set the default goal to that target either by placing "help" as the first target or by adding a .DEFAULT_GOAL := help line; update .PHONY to include help so it behaves like the other targets..ai/decisions.md (1)
80-80: Optional: split the multi-decision paragraph.Line 80 packs four distinct decisions into one paragraph (Sonar local vs Cloud target split, project-key/properties governance, token sourcing rules, and full-repo triage policy). Every other decision in this file gets its own
### …heading and short rationale. Splitting these into separate sections (e.g., "Sonar has explicit local and SonarCloud targets", "Sonar tokens come fromSONAR_TOKENor target-specific Keychain services", "Sonar findings are full-repository review signals") would make them linkable and easier to evolve independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ai/decisions.md at line 80, Split the long multi-decision paragraph that begins "Quality gates such as ruff, pytest, pyright, and SonarQube..." into three (or more) separate decision entries each with its own "### …" heading and short rationale: one titled "Sonar: explicit local and SonarCloud targets" describing local Docker vs GitHub CI target mappings (agentic-trader vs ogiboy_agentic-trader and sonar-project.properties vs CLI overrides), one titled "Sonar token sourcing" stating tokens must come from SONAR_TOKEN or target-specific Keychain services and that MCP wrappers inject tokens at process launch (not stored in editor config), and one titled "Sonar findings are full-repo review signals" describing triage/acceptance rules for security/correctness and blocker/critical maintainability findings; preserve the original wording for each rule but move them into their new titled sections for clarity and linkability.scripts/release/version_plan.py (1)
166-184:GITHUB_OUTPUTlines are not multi-line safe.Today the values are all single-line strings, so this works. But
key=valuewith a literal newline in the value silently breaks GH Actions output parsing (the runner requires thekey<<DELIM\n…\nDELIM\nform for multi-line values). If a future field (e.g., a free-form summary or release notes preview) ever flows throughpayload, this becomes a debugging trap. Cheap defensive guard:♻️ Defensive serialization
- for key, value in payload.items(): - lines.append(f"{key}={value}") + for key, value in payload.items(): + text_value = str(value) + if "\n" in text_value: + delimiter = f"EOF_{key.upper()}" + lines.append(f"{key}<<{delimiter}") + lines.append(text_value) + lines.append(delimiter) + else: + lines.append(f"{key}={text_value}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/release/version_plan.py` around lines 166 - 184, The _emit_github_outputs function currently writes key=value lines to GITHUB_OUTPUT which breaks if any payload value contains newlines; update _emit_github_outputs to detect values with newlines and emit them using the GitHub Actions multiline syntax (key<<DELIM\n...value...\nDELIM) using a safe delimiter (e.g., a generated token or commonly used EOF) for those entries while keeping single-line values as key=value; apply the same logic whether writing to the file at output_path or to sys.stdout and ensure all payload entries (constructed from payload/asdict(plan) and the boolean string fields) are processed through this serializer before writing.scripts/qa/run_sonar_scan.sh (1)
44-52: Optional: anchor perl regex compilation and tighten the redactor.Two small refinements: (1) without the
omodifier, perl recompiless/\Q$t\E/.../gfor every input line, which adds up on long scanner output; (2)redacted_log="${SCAN_LOG}"readsSCAN_LOGlazily — fine today, but a future refactor that callsredacted_runnerbefore the case-statement re-assignment (lines 136/140) would silently log to${ARTIFACT_DIR}/${SCANNER}.log(e.g.,py.log) instead of the intended file.♻️ Proposed tweak
- | SONAR_TOKEN_REDACT="${token_to_redact}" perl -pe 'BEGIN { $t = $ENV{SONAR_TOKEN_REDACT} // ""; } if (length $t) { s/\Q$t\E/<redacted>/g }' \ + | SONAR_TOKEN_REDACT="${token_to_redact}" perl -pe 'BEGIN { $t = $ENV{SONAR_TOKEN_REDACT} // ""; } if (length $t) { s/\Q$t\E/<redacted>/go }' \And consider setting
SCAN_LOGinside the case branch before declaring the default at line 9, so the source of truth is one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/qa/run_sonar_scan.sh` around lines 44 - 52, The redactor should compile the regex once and capture the intended log path eagerly: in redacted_runner, change the perl substitution to use the /o modifier (e.g., 's/\Q$t\E/<redacted>/go') so the regex is compiled once, and make redacted_log evaluate the current SCAN_LOG immediately with a safe fallback (e.g., local redacted_log="${SCAN_LOG:-${ARTIFACT_DIR}/${SCANNER}.log}") and optionally mark it readonly (local -r) so the log target isn’t affected by later reassignment; keep the SONAR_TOKEN -> SONAR_TOKEN_REDACT passing as-is to the perl env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai/tasks.md:
- Around line 259-261: Remove the duplicated `pnpm run qa:quality` bullet and
consolidate into one line that clearly states: "use `pnpm run qa:quality` for
terminal smoke plus quality checks", while keeping the other bullets unchanged
(`pnpm run qa`, `pnpm run sonar`, `pnpm run sonar:js`, `pnpm run sonar:cloud`)
and ensuring the note about Sonar paths emitting coverage XML and not writing
tokens to artifacts remains.
In `@agentic_trader/cli.py`:
- Around line 170-178: _tui_dependencies_installed currently treats any root
workspace node_modules (command_cwd/node_modules or its .pnpm) as sufficient and
can skip installing the TUI, causing missing deps; change the check so that the
function validates that the TUI-specific install exists by preferring
tui_dir/node_modules (i.e., require tui_dir / "node_modules" to exist) and, when
detecting a pnpm workspace (node_modules/.pnpm present under command_cwd),
require that tui_dir/node_modules also exists rather than treating the workspace
root as enough; update the boolean logic in _tui_dependencies_installed
(referencing function name and variables tui_dir and command_cwd and the
previous candidates) so only a true TUI-local node_modules satisfies the
installer skip condition.
In `@docs/app/`[lang]/layout.tsx:
- Around line 27-42: Create a single top-level root layout that owns the <html>
and <body> elements and move shared document-level attributes there (e.g. apply
jetbrainsMono.variable to the html class and global body classes); then remove
the <html> and <body> wrappers from the (landing) and [lang] segment layouts so
they only export segment-specific content. Specifically, add a root layout
exporting the document skeleton, and in the [lang] layout keep only the
RootProvider, children, and props like i18nUI.provider(lang) and search={{
options: { type: "static", api: searchApi } }}, removing duplicate html/body
elements and leaving segment logic (RootProvider, i18nUI.provider, searchApi)
intact.
In `@docs/components/feedback/client.tsx`:
- Around line 57-68: The storeFeedbackDraft function currently swallows all
storage errors which lets submitFeedback claim storedAt: "browser-local-storage"
even when persistence failed; change storeFeedbackDraft to return a boolean (or
throw) indicating actual success/failure (e.g., return true after
storage.setItem succeeds, false on caught errors) and update submitFeedback to
only set storedAt: "browser-local-storage" when storeFeedbackDraft returns true;
also ensure any other local-storage helpers (the logic referenced at lines
~91-97) follow the same pattern and do not hide errors so failures remain
visible to the caller.
In `@docs/lib/home/content/en.ts`:
- Around line 130-132: The "Feedback flow" card body text is ambiguous; update
the body string in the object with title "Feedback flow" to replace the fragment
"server-side forwarding should only return if a Node-hosted docs surface is
explicit" with a clear, consistent sentence (for example: "server-side
forwarding should only occur when the docs site is explicitly hosted on a Node
server") so it matches the wording used in
docs/content/docs/en/memory-and-review.mdx and removes the ambiguous word
"return."
In `@docs/next.config.mjs`:
- Around line 2-8: Add a Node engine constraint to prevent import.meta.dirname
runtime errors by updating both the root package.json and docs/package.json to
include an "engines" field with "node": ">=22"; locate the package.json files
(root and docs) and add/merge the "engines": { "node": ">=22" } entry, ensure
JSON remains valid, and commit the change so local development matches CI's Node
22 requirement.
In `@scripts/qa/run_sonar_scan.sh`:
- Around line 143-146: The error message for the default case in the SCANNER
case-switch understates the accepted scanner aliases; update the message printed
in the default branch (where it currently echoes "Unknown scanner '${SCANNER}'.
Use 'pysonar' or 'npm'.") to list all valid aliases accepted by the case (py,
python, pysonar, js, node, npm, sonar) so users see the full set of accepted
scanner names when an unknown value is passed.
- Around line 108-115: run_npm_scanner currently always appends the
-Dsonar.python.coverage.reportPaths=${COVERAGE_XML} arg even when run_coverage
may have skipped generating the file (SONAR_SKIP_COVERAGE), causing the npm
scanner to point at a missing file; change run_npm_scanner to conditionally add
the coverage argument only if the COVERAGE_XML file exists (same guard used in
run_pysonar), i.e., check [[ -f "${COVERAGE_XML}" ]] before appending that
"-Dsonar.python.coverage.reportPaths=${COVERAGE_XML}" element to the local
command array.
- Around line 80-89: The SONAR token is being passed on the command line and
resolve_token() only exports SONAR_TOKEN when fetched from Keychain; update
resolve_token() to always export SONAR_TOKEN (whether provided by caller or
retrieved) so the env var is set consistently, then remove the --sonar-token
"${SONAR_TOKEN}" entry from the command array construction (the CLI will read
SONAR_TOKEN from the environment). Ensure you only modify resolve_token() to
export the variable and delete the --sonar-token flag in the block that builds
the command (the command+=(...) array).
In `@scripts/release/version_plan.py`:
- Around line 94-100: The _build_number function silently falls back to 1 when
neither the provided raw value nor git commit count are available; modify
_build_number to emit a visible warning (e.g., using the module logger or print
to stderr) when it must use the final fallback value so callers can detect the
shallow/detached-git situation. Locate the function _build_number and the call
to _run_git in this file and add a one-line warning that includes context
(function name and reason: missing GITHUB_RUN_NUMBER and git rev-list count)
before returning 1 so the failure mode is visible to CI logs or local runs.
- Around line 74-86: The normalization is asymmetric: update SEMVER_RE to accept
an optional uppercase or lowercase prefix (use ^[Vv]? or make the regex
case-insensitive) and change _normalize_semver to strip either "v" or "V" (e.g.,
remove leading [Vv]) before returning the normalized string so tags like
"V0.9.5" are accepted; ensure the emitted/returned value always uses the
lowercase "v" convention (or no "v" per project convention) so downstream checks
like _semver_core(…) and the post-build guard (next/beta checks) validate the
same normalized form — also ensure callers pass the normalized string into
_semver_core to keep validation uniform.
In `@scripts/secrets/run-sonarqube-mcp.sh`:
- Around line 23-26: The script currently only exports SONARQUBE_TOKEN inside
the conditional that fetches it from Keychain, so if a caller provided
SONARQUBE_TOKEN as a non-exported shell variable the script skips the fetch and
never exports it; change the logic in run-sonarqube-mcp.sh to always export
SONARQUBE_TOKEN (move export SONARQUBE_TOKEN outside the if block) while keeping
the existing fetching behavior using KEYCHAIN_GET and
SONARQUBE_KEYCHAIN_SERVICE/SONARQUBE_KEYCHAIN_ACCOUNT when SONARQUBE_TOKEN is
empty.
In `@tests/test_backtest.py`:
- Around line 41-45: In _index_iso replace the unnecessary getattr call with
direct attribute access: obtain the Timestamp via
frame.index.to_list()[position] and call its isoformat() method directly (i.e.,
use value.isoformat()). Update the function _index_iso to remove getattr usage
so it directly invokes isoformat on the Timestamp.
In `@tests/test_features.py`:
- Around line 86-87: Replace the dynamic getattr call with direct attribute
access: instead of calling getattr(expected_as_of, "isoformat")(), call
expected_as_of.isoformat() so the assertion becomes snapshot.as_of ==
str(expected_as_of.isoformat()); update the assertion that uses expected_as_of
(from index.to_list()[-2]) accordingly to remove the getattr usage.
In `@webgui/src/app/api/chat/route.ts`:
- Around line 41-50: The parsed request body is cast to { persona?: string;
message?: string } but field types aren't validated, so non-string body.message
causes body.message?.trim() to throw; update the validation after parsing (the
parsed/ body variables in route.ts) to explicitly check typeof parsed.message
=== 'string' (and optional parsed.persona === 'string' if provided) before
assigning/using body, and if not a string return Response.json({ error: 'invalid
message' }, { status: 400 }); ensure any subsequent use of body.message (e.g.,
calling trim()) is only performed after this type guard so malformed clients get
a 400 instead of a 500.
---
Outside diff comments:
In `@scripts/qa/smoke_qa.py`:
- Around line 421-429: The CheckResult currently embeds the raw exception object
in the details field which leaks sensitive data; replace the unredacted exc with
the already-redacted exception_text (produced by _redact_sensitive_text) when
constructing the CheckResult (the code path that calls CheckResult(name=name,
passed=False, details=..., artifact=...)), ensuring details uses exception_text
(string) rather than exc; reference symbols: _redact_sensitive_text,
exception_text, _write_artifact, and CheckResult so you update the details
assignment to include the redacted text.
In `@webgui/src/app/api/instruct/route.ts`:
- Around line 43-55: The handler currently uses body.message?.trim() which will
throw for non-string values and escalate to a 500; update the request validation
in route.ts to explicitly check types before calling trim: confirm typeof
body.message === 'string' and that body.message.trim() is non-empty, and
validate body.apply with typeof body.apply === 'boolean' (or undefined) before
using it; return a 400 Response.json({ error: 'invalid request' }, { status: 400
}) for bad types so malformed client input stays on the 4xx path rather than
causing an exception in the outer try.
---
Nitpick comments:
In @.ai/decisions.md:
- Line 80: Split the long multi-decision paragraph that begins "Quality gates
such as ruff, pytest, pyright, and SonarQube..." into three (or more) separate
decision entries each with its own "### …" heading and short rationale: one
titled "Sonar: explicit local and SonarCloud targets" describing local Docker vs
GitHub CI target mappings (agentic-trader vs ogiboy_agentic-trader and
sonar-project.properties vs CLI overrides), one titled "Sonar token sourcing"
stating tokens must come from SONAR_TOKEN or target-specific Keychain services
and that MCP wrappers inject tokens at process launch (not stored in editor
config), and one titled "Sonar findings are full-repo review signals" describing
triage/acceptance rules for security/correctness and blocker/critical
maintainability findings; preserve the original wording for each rule but move
them into their new titled sections for clarity and linkability.
In @.github/pull_request_template.md:
- Around line 9-12: Update the "## Testing" section in the PR template by
replacing the placeholder "-" with a short, structured prompt (for example:
`command`, `expected result`, `evidence path`) so reviewers can reproduce
validations; change the content under the "## Testing" header (referenced as the
"## Testing" block) to require those three fields and apply the same change to
the other occurrences noted (lines 22-23 equivalent sections) to keep testing
instructions consistent and auditable.
In @.gitignore:
- Line 52: Replace the narrow ignore pattern "*/out/" in .gitignore with a
recursive rule so root-level and nested out/ directories are ignored; locate the
current "*/out/" entry and change it to a recursive pattern such as "**/out/"
(or simply "out/") to ensure all out/ directories are covered.
In `@agentic_trader/agents/fundamental.py`:
- Around line 279-308: Add concise one-line docstrings to _fallback_risk_flags,
_fallback_strengths, and _has_provider_gap describing their purpose and return
values; for _has_provider_gap explicitly note that a missing context or missing
context.decision_features is treated as a provider gap (returns True). Keep each
docstring short and placed immediately under the def line for the corresponding
functions (_fallback_risk_flags, _fallback_strengths, _has_provider_gap).
- Around line 420-430: The function _has_structured_fundamental_evidence
duplicates the set of provider-gap flags; replace the local missing_flags set
with the module-level constant PROVIDER_GAP_FLAGS to avoid drift. Update
_has_structured_fundamental_evidence to use PROVIDER_GAP_FLAGS (keeping the
existing behavior of checking intersection with flags from
context.decision_features.fundamental.quality_flags) and remove the hard-coded
missing_flags variable so the function references the single source of truth.
In `@agentic_trader/market/features.py`:
- Around line 95-97: The helper _as_float contains a redundant typing.cast call;
replace the implementation of _as_float to remove the inner cast and simply
convert the input to float (i.e. use float(value)) so the runtime is not doing a
no-op cast — update the function named _as_float accordingly so the type-checker
still sees it as a float conversion but the code uses float(value) directly.
In `@docs/app/`(landing)/layout.tsx:
- Around line 1-3: Summary: Normalize quote style in layout.tsx to match the
repo's preferred style. Update the import/export statements in the file so all
string literals use the repo-preferred quotes (e.g., change 'next/font/google',
'../globals.css', and '@/lib/site-metadata' to the consistent quote style used
across the codebase) by editing the top-level import of JetBrains_Mono and the
imports/exports so they use the same quote character; ensure references to
JetBrains_Mono and the exported docsMetadata as metadata remain unchanged.
In `@docs/components/feedback/copy.ts`:
- Around line 13-17: Rename the stale keys in the feedback copy type and all
consumers: change successForwarded -> successPrepared and openDiscussion ->
openIssue in docs/components/feedback/copy.ts, then update every usage that
reads those properties (notably the forwarding === "prepared" branch in
feedback-result.tsx and any test fixtures) to use successPrepared and openIssue;
ensure TypeScript types and exported object keys match and run tests to pick up
any remaining references.
In `@docs/lib/site-metadata.ts`:
- Around line 3-5: The basePath value read from NEXT_PUBLIC_BASE_PATH can
contain a trailing slash which causes double-slashes when assetPath concatenates
paths; normalize basePath in docs/lib/site-metadata.ts by trimming any trailing
slashes and ensuring either an empty string or a single leading slash (e.g.,
strip /+ from the end and guarantee a leading slash only when non-empty) before
using it in assetPath so assetPath('/favicon.ico') never produces a
double-slash; update the basePath initialization (symbol: basePath) so assetPath
(symbol: assetPath) concatenation is safe regardless of how
NEXT_PUBLIC_BASE_PATH is set.
In `@Makefile`:
- Around line 1-82: Add a discoverable default help target to the Makefile and
make it the default goal: create a "help" target (and add "help" to the .PHONY
list) that prints a brief list of the existing alias targets (e.g., setup,
check, build, qa, sonar, webgui, docs, tui, clean), then set the default goal to
that target either by placing "help" as the first target or by adding a
.DEFAULT_GOAL := help line; update .PHONY to include help so it behaves like the
other targets.
In `@scripts/check-python.sh`:
- Around line 19-20: The hardcoded fallback invoking /opt/anaconda3/bin/pyright
is developer-machine specific and should be removed or made discoverable; change
the elif branch in scripts/check-python.sh so it either drops this hardcoded
path (relying on the existing command -v pyright path check) or searches common
conda/miniconda prefixes (e.g., $HOME/anaconda3, $HOME/miniconda3,
/opt/miniconda3) for a pyright binary and then invoke it with the existing
arguments (use the same ${PYRIGHT_TARGETS} and --pythonpath "${PYTHON_EXEC}"
when found); ensure you still fall back to the current error branch if no
pyright is discovered.
- Around line 16-20: The shellcheck warning SC2086 about unquoted
${PYRIGHT_TARGETS} is intentional because it must split into multiple argv
entries; suppress the warning by adding a shellcheck pragma (e.g. a comment "#
shellcheck disable=SC2086") immediately above the pyright invocations that use
${PYRIGHT_TARGETS} (the three branches calling "poetry run pyright
${PYRIGHT_TARGETS}", "pyright --pythonpath \"${PYTHON_EXEC}\"
${PYRIGHT_TARGETS}", and "/opt/anaconda3/bin/pyright --pythonpath
\"${PYTHON_EXEC}\" ${PYRIGHT_TARGETS}"); keep the rest of the script
POSIX-compatible and do not change quoting for ${PYRIGHT_TARGETS}.
In `@scripts/qa/run_sonar_scan.sh`:
- Around line 44-52: The redactor should compile the regex once and capture the
intended log path eagerly: in redacted_runner, change the perl substitution to
use the /o modifier (e.g., 's/\Q$t\E/<redacted>/go') so the regex is compiled
once, and make redacted_log evaluate the current SCAN_LOG immediately with a
safe fallback (e.g., local
redacted_log="${SCAN_LOG:-${ARTIFACT_DIR}/${SCANNER}.log}") and optionally mark
it readonly (local -r) so the log target isn’t affected by later reassignment;
keep the SONAR_TOKEN -> SONAR_TOKEN_REDACT passing as-is to the perl env.
In `@scripts/release/preview_version_plan.sh`:
- Line 13: Replace the POSIX test bracket in the release preview conditional
with Bash's safer conditional syntax: change the conditional that currently
reads `if [ "$status" -ne 0 ]; then` to use `[[ ... ]]` so it becomes `if [[
"$status" -ne 0 ]]; then`, ensuring no word-splitting or glob expansion issues
when evaluating the `status` variable in the script `preview_version_plan.sh`.
- Around line 4-9: Replace the fixed stderr path
/tmp/semantic-release-preview.log with a secure temporary file created via
mktemp, redirect semantic-release's stderr to that temp file, and ensure the
temp file is removed after use; update the pipeline around the semantic-release
invocation (the subshell assigning tag from the poetry run semantic-release ...
| grep ... | tail -n 1 sequence) to use the temp file and preserve the existing
fallback behavior for empty grep matches (so tag stays "" if no match), and
consider keeping the set +e around that subshell or explicitly swallowing
non-zero exit from grep (e.g., handle grep returning 1) so pipefail doesn't
unexpectedly abort the script.
In `@scripts/release/version_plan.py`:
- Around line 166-184: The _emit_github_outputs function currently writes
key=value lines to GITHUB_OUTPUT which breaks if any payload value contains
newlines; update _emit_github_outputs to detect values with newlines and emit
them using the GitHub Actions multiline syntax (key<<DELIM\n...value...\nDELIM)
using a safe delimiter (e.g., a generated token or commonly used EOF) for those
entries while keeping single-line values as key=value; apply the same logic
whether writing to the file at output_path or to sys.stdout and ensure all
payload entries (constructed from payload/asdict(plan) and the boolean string
fields) are processed through this serializer before writing.
In `@scripts/secrets/install-sonarqube-mcp-wrapper.sh`:
- Around line 11-28: The heredoc used to write the wrapper (cat
>"${INSTALL_PATH}" <<EOF) allows shell expansion of ${CANONICAL_SCRIPT} and
${FALLBACK_SCRIPT} at install time, which can break the generated script if
those values contain special characters; change the write so the heredoc is
quoted (e.g. <<'EOF') to prevent interpolation, then explicitly inject the
desired values afterward (or perform a safe sed substitution) into the marker
variables AGENTIC_TRADER_SONAR_MCP_SCRIPT/CANONICAL_SCRIPT/FALLBACK_SCRIPT
within the file; locate the write block around INSTALL_PATH and update the
heredoc quoting and post-write substitution logic to safely embed those paths.
In `@scripts/secrets/run-sonarqube-mcp.sh`:
- Line 9: The SONARQUBE_KEYCHAIN_ACCOUNT assignment may produce an empty value
when neither SONARQUBE_KEYCHAIN_ACCOUNT nor USER is set, causing downstream
calls like keychain-get.sh "${service}" "" to fail cryptically; after the
existing assignment to SONARQUBE_KEYCHAIN_ACCOUNT in run-sonarqube-mcp.sh, add
an explicit check that the variable is non-empty and, if empty, print a clear
error mentioning SONARQUBE_KEYCHAIN_ACCOUNT and exit non‑zero (so callers see a
fast, descriptive failure instead of passing an empty account to
keychain-get.sh).
In `@scripts/sonarqube/start-local.sh`:
- Around line 7-12: Check that the script verifies both the Docker CLI and the
Docker Compose capability and that the compose file exists: detect whether
"docker compose" is supported (fallback to "docker-compose" if present) before
running the compose command, and validate the COMPOSE_FILE (and any override via
SONARQUBE_COMPOSE_FILE) path is present and readable; if checks fail, print
clear error messages and exit non‑zero instead of invoking docker compose with
an invalid configuration. Ensure you update the script around the existing
docker check and the docker compose invocation (references: COMPOSE_FILE,
SONARQUBE_COMPOSE_FILE, and the "docker compose" / "docker-compose" commands).
In `@scripts/sonarqube/status-local.sh`:
- Around line 7-9: The docker ps command uses two name filters (--filter
"name=sonarqube" and --filter "name=sonarqube-db") which is redundant because
name= matches substrings and multiple name filters are ORed; fix by either
removing the unnecessary --filter "name=sonarqube" or --filter
"name=sonarqube-db" to list only the desired container, or make the match strict
by anchoring the regex (e.g., --filter "name=^sonarqube$" and/or --filter
"name=^sonarqube-db$") so each filter matches exactly the intended container
names.
In `@tests/test_backtest.py`:
- Around line 41-45: Duplicate ISO-formatting of index values is present in
_index_iso (tests/test_backtest.py) and in tests/test_features.py; extract this
helper into a shared test helper (e.g., add a function iso_index or _index_iso
in tests/conftest.py or tests/_helpers.py) and update both tests to import and
call that shared helper instead of duplicating the code, ensuring the helper
accepts a DataFrame and position and returns str(value.isoformat()) so existing
assertions keep working.
In `@webgui/src/app/api/chat/route.ts`:
- Around line 41-50: Extract the repeated JSON parsing logic into a shared
helper named parseJsonObjectBody that takes a Request and returns either { ok:
true; body: Record<string, unknown> } or { ok: false; response: Response };
replace the try/ catch + typeof/null check (the block that calls request.json(),
validates typeof object && !== null and returns Response.json({ error: 'invalid
json' }, { status: 400 })) in chat route handler with a call to this helper and
early-return the helper.response when ok is false, then cast the returned body
to your existing local shape (e.g., the body variable used in chat/route.ts) and
apply the same replacement in instruct/route.ts, runtime/route.ts and
dashboard/route.ts so all request.json() parsing is centralized and can be
tightened (e.g., reject Array.isArray) in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
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
Validation
Notes