Skip to content

fix: restore clean web validation for make all#337

Merged
yacosta738 merged 9 commits into
feat/258-expand-mcp-platform-support-beyond-tools-only-integrationfrom
fix/sonarqube-code-smells
Mar 27, 2026
Merged

fix: restore clean web validation for make all#337
yacosta738 merged 9 commits into
feat/258-expand-mcp-platform-support-beyond-tools-only-integrationfrom
fix/sonarqube-code-smells

Conversation

@yacosta738
Copy link
Copy Markdown
Contributor

No description provided.

- Replace window with globalThis across useChat.ts, useGateway.ts, useConfig.ts
- Move inner functions to module scope (createSessionId, trimSlashes, etc.)
- Replace charCodeAt with codePointAt in useGateway.ts
- Refactor nested ternaries in onboarding.ts into helper function
- Reduce cognitive complexity in validate-docs-metadata.mjs via extracted helpers
- Use String.raw and replaceAll in validate-docs-metadata.mjs
- Add explicit return 0 in check-tools.sh
- ~41 SonarQube issues resolved
- sandbox.rs: 86→~5 — extract 11 path validation helpers
- channels/mod.rs: 67→~7 — extract 8 message processing stage helpers
- skills/mod.rs: 31→~5, 36→~8 — extract skill loading and install helpers
- scanner.rs: 35→~3 — extract pattern scanning helpers
- lockfile.rs: 24→~3 — extract disk reconciliation helpers
- telegram.rs: 26→~10 — extract content part building helpers
- All 6,132 tests passing, clippy clean, fmt clean
Add the missing @vue/compiler-dom dependency required by Vue component coverage tests and remove a non-null assertion in useConfig so the web workspace passes check and coverage during make all.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • main
  • develop

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0dd66874-d79c-463a-abf7-e48bd4178d9f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sonarqube-code-smells

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…tools-only-integration' into fix/sonarqube-code-smells

# Conflicts:
#	clients/web/pnpm-lock.yaml
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Mar 27, 2026

Deploying corvus with  Cloudflare Pages  Cloudflare Pages

Latest commit: d62854a
Status: ✅  Deploy successful!
Preview URL: https://a14bf30c.corvus-42x.pages.dev
Branch Preview URL: https://fix-sonarqube-code-smells.corvus-42x.pages.dev

View logs

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 27, 2026

✅ Contributor Report

User: @yacosta738
Status: Passed (12/13 metrics passed)

Metric Description Value Threshold Status
PR Merge Rate PRs merged vs closed 89% >= 30%
Repo Quality Repos with ≥100 stars 0 >= 0
Positive Reactions Positive reactions received 9 >= 1
Negative Reactions Negative reactions received 0 <= 5
Account Age GitHub account age 3073 days >= 30 days
Activity Consistency Regular activity over time 108% >= 0%
Issue Engagement Issues with community engagement 0 >= 0
Code Reviews Code reviews given to others 457 >= 0
Merger Diversity Unique maintainers who merged PRs 2 >= 0
Repo History Merge Rate Merge rate in this repo 91% >= 0%
Repo History Min PRs Previous PRs in this repo 182 >= 0
Profile Completeness Profile richness (bio, followers) 90 >= 0
Suspicious Patterns Spam-like activity detection 1 N/A

Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-27 to 2026-03-27

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
clients/agent-runtime/src/skills/sandbox.rs (1)

268-389: ⚠️ Potential issue | 🔴 Critical

Confirm that required Rust validation checks have been run

Per coding guidelines, this PR must run cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, and cargo test for all code in clients/agent-runtime/, or document which checks were skipped and why. Please confirm the results of running these checks.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clients/agent-runtime/src/skills/sandbox.rs` around lines 268 - 389, Run the
required validation checks for the agent-runtime crate: from the
clients/agent-runtime directory run `cargo fmt --all -- --check`, `cargo clippy
--all-targets -- -D warnings`, and `cargo test` and fix any failures (formatting
issues, clippy warnings, or failing tests such as those in the tests:: module
referencing validate_tool_paths and build_policy); if you cannot run any of
these, document explicitly which check was skipped and why and include the exact
command output for each check in the PR so reviewers can verify the state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clients/agent-runtime/src/channels/mod.rs`:
- Around line 788-797: The helper extract_user_text should prefer the canonical
msg.content before recomputing text from parts to avoid duplicated captions;
change its logic in function extract_user_text to first return msg.content if it
is non-empty, and only call/use msg.text_projection() (and return that) when
msg.content is empty, ensuring you still handle the empty-parts case
consistently.

In `@clients/agent-runtime/src/channels/telegram.rs`:
- Around line 24-60: The caption is only added as a ContentPart::Text inside the
photo/allowed-image-document branches, so non-image captioned attachments
(PDF/video/audio) produce no text part; modify the telegram parsing to push the
extracted caption (the local variable caption) into parts as a ContentPart::Text
unconditionally (e.g., immediately after caption is computed and before
photo/document handling) so captions are always preserved; ensure you keep
existing logic that also adds caption when creating Image parts, update
references in build_document_image_part and anywhere handle_unauthorized_message
expects text so bind/approval hints still fire, and add a regression test that
sends a captioned non-image media (e.g., PDF/video) verifying the caption
appears in parts and unauthorized handling behaves correctly.

In `@clients/agent-runtime/src/skills/lockfile.rs`:
- Around line 194-234: The function reconcile_skill_entry currently reads the
file twice; instead, read the file once into an Option<Vec<u8>> (e.g., let
bytes_opt = std::fs::read(skill_md).ok()), compute current_hash from bytes_opt
once, and then reuse bytes_opt when handling the existing entry (compare
compute_content_hash(&bytes) only if bytes_opt.is_some()); remove the second
std::fs::read call inside the existing branch and use bytes_opt to decide
whether to update existing.content_hash/installed_at or increment
summary.unchanged, and keep using current_hash when creating the new LockEntry
so the code never re-reads the file.

In `@clients/agent-runtime/src/skills/sandbox.rs`:
- Around line 155-158: The warning currently logs the raw incoming path variable
arg via tracing::warn!; change it to avoid emitting raw/untrusted input by
replacing the message with a generic statement (e.g., "cannot canonicalize path
— denying access") and, if you need correlation, log a redacted or stable
fingerprint instead (compute a non-reversible hash or truncate/redact arg and
log that value). Update the tracing::warn! call so it never interpolates arg
directly but uses the redacted/hash string or a constant placeholder.

In `@clients/web/apps/dashboard/src/composables/useConfig.ts`:
- Around line 689-693: The 409 conflict path in saveSection bypasses the
tolerant JSON parser and directly calls response.json(), which can throw on
empty/non-JSON bodies; replace the direct call with the existing tolerant helper
readJsonPayload(response) and then extract fields safely (e.g., const conflict =
(await readJsonPayload(response)) as { fields?: string[] } and keep the
Array.isArray check) so errorMessage.value is set to t("form.restartRequired", {
fields }) when appropriate without throwing.

In `@scripts/validate-docs-metadata.mjs`:
- Around line 218-223: The function collectFrontmatterLinkSlugs treats any
non-http/https target as internal (so mailto: and other schemes are incorrectly
considered internal), causing false orphan detection; update
collectFrontmatterLinkSlugs to use the same external-target guard used elsewhere
by rejecting targets that match a URL scheme (e.g., patterns like
/^[a-zA-Z][a-zA-Z0-9+.-]*:/) or otherwise explicitly checking for common schemes
(mailto:, ftp:, tel:, etc.) before adding to referenced, so only true internal
paths (not scheme-prefixed targets) are normalized and added to the referenced
set.
- Around line 203-207: The current candidate probe (variables resolved,
candidates, existingCandidate using existsSync/statSync) can follow relative
targets outside the intended docsRoot; before iterating candidates or calling
existsSync/statSync, validate that each resolved path stays inside the docsRoot
boundary (e.g., compute path.resolve for the candidate and ensure
path.relative(docsRoot, candidateResolved) does not start with '..' or otherwise
confirm candidateResolved.startsWith(docsRoot + path.sep)); reject or skip any
candidate that fails this containment check to prevent path traversal prior to
filesystem probing.

---

Outside diff comments:
In `@clients/agent-runtime/src/skills/sandbox.rs`:
- Around line 268-389: Run the required validation checks for the agent-runtime
crate: from the clients/agent-runtime directory run `cargo fmt --all --
--check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` and fix
any failures (formatting issues, clippy warnings, or failing tests such as those
in the tests:: module referencing validate_tool_paths and build_policy); if you
cannot run any of these, document explicitly which check was skipped and why and
include the exact command output for each check in the PR so reviewers can
verify the state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2e56259f-76ef-4a2f-b3d2-769f14397e57

📥 Commits

Reviewing files that changed from the base of the PR and between a0f8b17 and 206dd9c.

⛔ Files ignored due to path filters (1)
  • clients/web/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (16)
  • clients/agent-runtime/src/channels/mod.rs
  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/scanner.rs
  • clients/web/apps/chat/package.json
  • clients/web/apps/chat/src/composables/useChat.ts
  • clients/web/apps/chat/src/composables/useGateway.ts
  • clients/web/apps/dashboard/package.json
  • clients/web/apps/dashboard/src/composables/useConfig.ts
  • clients/web/package.json
  • clients/web/packages/shared/onboarding.ts
  • clients/web/pnpm-workspace.yaml
  • scripts/check-tools.sh
  • scripts/validate-docs-metadata.mjs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (5)
**/*

⚙️ CodeRabbit configuration file

**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.

Files:

  • clients/web/apps/dashboard/package.json
  • clients/web/pnpm-workspace.yaml
  • scripts/check-tools.sh
  • clients/web/apps/chat/package.json
  • clients/web/package.json
  • clients/web/apps/chat/src/composables/useChat.ts
  • clients/web/apps/chat/src/composables/useGateway.ts
  • scripts/validate-docs-metadata.mjs
  • clients/web/packages/shared/onboarding.ts
  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/web/apps/dashboard/src/composables/useConfig.ts
  • clients/agent-runtime/src/skills/scanner.rs
  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
clients/agent-runtime/src/**/*.rs

📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)

clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency

Files:

  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/scanner.rs
  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
clients/agent-runtime/**/*.rs

📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)

Run cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, and cargo test for code validation, or document which checks were skipped and why

Files:

  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/scanner.rs
  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
**/*.rs

⚙️ CodeRabbit configuration file

**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.

Files:

  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/scanner.rs
  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
clients/agent-runtime/src/channels/**/*.rs

📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)

Implement Channel trait in src/channels/ with consistent send, listen, and health_check semantics and cover auth/allowlist/health behavior with tests

Files:

  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
🧠 Learnings (7)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why

Applied to files:

  • clients/agent-runtime/src/skills/lockfile.rs
  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/scanner.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path

Applied to files:

  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths

Applied to files:

  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/channels/telegram.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks

Applied to files:

  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
  • clients/agent-runtime/src/skills/scanner.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable

Applied to files:

  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/skills/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow

Applied to files:

  • clients/agent-runtime/src/skills/sandbox.rs
  • clients/agent-runtime/src/channels/mod.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests

Applied to files:

  • clients/agent-runtime/src/channels/telegram.rs
  • clients/agent-runtime/src/channels/mod.rs
🔇 Additional comments (29)
scripts/check-tools.sh (1)

22-22: Good hardening of function exit behavior.

Explicitly returning success in numeric_prefix() is a solid safeguard under set -euo pipefail and helps prevent brittle failures from future edits.

As per coding guidelines, "Security first, performance second... Look for behavioral regressions...," this change reduces regression risk in version parsing flow.

clients/web/pnpm-workspace.yaml (1)

15-15: Catalog pin is consistent and safe.

Line 15 correctly centralizes @vue/compiler-dom at 3.5.30, matching the Vue toolchain versioning strategy and avoiding cross-package drift.

As per coding guidelines, "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. Look for behavioral regressions, missing tests, and contract breaks across modules."

clients/web/apps/chat/package.json (1)

39-39: Good devDependency alignment with workspace catalog.

Line 39 is a clean addition and keeps chat app build tooling consistent without changing runtime dependencies.

As per coding guidelines, "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. Look for behavioral regressions, missing tests, and contract breaks across modules."

clients/web/apps/dashboard/package.json (1)

40-40: Dashboard toolchain update looks correct.

Line 40 properly adds @vue/compiler-dom through the shared catalog, with no runtime contract or security impact.

As per coding guidelines, "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. Look for behavioral regressions, missing tests, and contract breaks across modules."

clients/web/package.json (1)

32-32: Root dependency management remains coherent.

Line 32 is a solid addition for consistent workspace tooling and does not introduce runtime-facing risk.

As per coding guidelines, "Security first, performance second. Validate input boundaries, auth/authz implications, and secret management. Look for behavioral regressions, missing tests, and contract breaks across modules."

clients/agent-runtime/src/skills/sandbox.rs (1)

70-92: Good refactor: clearer fail-closed path validation flow

Nice extraction into validate_single_path with explicit branching for symlink/existing/nonexistent paths; behavior remains deny-first and easier to audit.

scripts/validate-docs-metadata.mjs (1)

336-371: Good extraction of lastReviewed validation.

Splitting parsing/age checks into validateReviewDate + validateLastReviewed improves readability and keeps the validation path focused.

clients/agent-runtime/src/skills/scanner.rs (5)

48-60: LGTM — clean separation of concerns.

The main scan function now clearly orchestrates per-line checks and multi-line block detection via dedicated helpers. This improves readability without changing the scoring behavior.


62-88: LGTM — centralizing patterns as constants.

Moving pattern lists to const statics improves maintainability and makes future additions straightforward.


90-149: LGTM — scan_line_patterns correctly aggregates all per-line checks.

The helper cleanly checks all pattern categories, single-line base64, and unicode anomalies in a single pass per line. The to_lowercase() call is appropriate for case-insensitive matching.


151-178: LGTM — helper extraction and base64 detection.

check_patterns and is_base64_like are focused helpers with no unnecessary allocations beyond the required ScanFinding creation.


180-220: LGTM — multi-line base64 detection with proper trailing-block handling.

The scan_multiline_base64 and emit_multiline_b64 helpers correctly accumulate contiguous base64-like lines and emit findings when the total exceeds 200 chars. The trailing block check at line 201 ensures blocks at EOF are not missed.

clients/agent-runtime/src/skills/lockfile.rs (3)

145-161: LGTM — repair_lockfile refactored with preserved atomic write.

The orchestration cleanly delegates to helpers and maintains the atomic temp-file-then-rename pattern for lockfile updates.


163-192: LGTM — collect_disk_skills handles missing directory gracefully.

Early return for non-existent skills_path is correct, and directory iteration properly filters for directories containing SKILL.md.


236-252: LGTM — orphan removal is straightforward.

remove_orphaned_entries correctly identifies and removes lockfile entries for skills no longer on disk.

clients/agent-runtime/src/skills/mod.rs (9)

71-84: LGTM — load_skills_with_config cleanly delegates to helpers.

The per-skill processing loop now reads clearly: enrich from lockfile, verify integrity, scan for injection, then set sandbox flags. Security posture is preserved.


86-96: LGTM — enrich_skill_from_lockfile correctly enriches trust/origin.

Skills without lockfile entries retain default Local trust, maintaining secure-by-default behavior.


122-150: LGTM — trust-aware integrity mismatch logging.

Third-party skills correctly have tools disabled on mismatch, while Official and Local only log warnings. This preserves the defense-in-depth model.


152-177: LGTM — scan_third_party_skill with appropriate guards.

The early returns for non-ThirdParty skills, missing threshold, and empty content are correct. Tool disablement on threshold breach maintains security.


912-946: LGTM — validate_and_parse_skill_md with cleanup on failure.

Directory removal on validation failure prevents orphaned directories. Name mismatch check is a good defensive measure.


948-997: LGTM — scan_and_gate_install blocks suspicious content unless explicitly trusted.

The --trust flag allows override with a logged warning, which is appropriate for power users. Findings are reported before aborting.


999-1030: LGTM — gate_trust_consent requires explicit approval for third-party tools.

Interactive confirmation in terminal mode, hard failure in non-interactive mode without --trust. This is secure-by-default.


1032-1069: LGTM — write_install_lock_entry follows advisory model.

Lockfile write failures are logged but don't block installation, consistent with the advisory lockfile design (AD2).


109-112: 🧹 Nitpick | 🔵 Trivial

Avoid unnecessary clone on skill.location.

Using .clone().unwrap_or_else(...) forces an allocation even when the value is Some. Use as_ref() with map_or_else() or match instead.

♻️ Proposed refactor
-    let skill_md_path = skill
-        .location
-        .clone()
-        .unwrap_or_else(|| skills_path.join(&skill.name).join("SKILL.md"));
+    let skill_md_path = skill
+        .location
+        .as_ref()
+        .cloned()
+        .unwrap_or_else(|| skills_path.join(&skill.name).join("SKILL.md"));

Or if the intent is to derive path from location:

+    let skill_md_path = match &skill.location {
+        Some(loc) => loc.clone(),
+        None => skills_path.join(&skill.name).join("SKILL.md"),
+    };
			> Likely an incorrect or invalid review comment.
clients/web/packages/shared/onboarding.ts (3)

17-26: Clean extraction of step status resolution logic.

The helper correctly prioritizes: complete → pending (if prerequisite unmet) → blocked → current. This eliminates duplicate conditionals across steps.


43-70: Step prerequisite and blocked condition mappings look correct.

  • runtime: No prerequisite (first step), blocked on infra issues.
  • trust: Requires runtimeConfirmed, blocked on credential/input failures.
  • connect: Requires trustEstablished, blocked on pairing issues.

The chained dependencies enforce proper sequencing.


76-76: No blockedRecovery values currently map to the final step.

All defined recovery kinds (runtime_unavailable, transport_unavailable, trust_input_invalid, trust_input_expired, credential_missing, credential_invalid, paired_but_not_connected) explicitly target runtime, trust, or connect steps. The final step hardcoding isBlocked = false is consistent with the current design—there are no conditional checks for blocking the final step anywhere in the codebase, and no tests validate final step blocking behavior.

If this was a behavioral regression from previous code, it cannot be verified without access to the original implementation or PR context. Confirm whether the final step should ever block before flagging as a bug.

clients/web/apps/chat/src/composables/useChat.ts (1)

107-137: Add one non-DOM regression test for the storage helpers.

The new behavior here is the typeof globalThis.window === "undefined" path, but clients/web/apps/chat/src/composables/useChat.spec.ts:1-20 and 48-68 still only exercise browser window.sessionStorage. One SSR/no-DOM test would keep this compatibility branch from regressing silently. As per coding guidelines, "Look for behavioral regressions, missing tests, and contract breaks across modules."

clients/web/apps/chat/src/composables/useGateway.ts (1)

214-218: No guard needed—this app is browser-only, not SSR-compatible.

The gatewayUrl() function dereferences globalThis.location.origin without a guard, but the chat app has no SSR or Node.js runtime configuration. It runs exclusively in the browser, so the default /api path will never throw in SSR/Node contexts. The matching pattern in useConfig.ts has the same behavior and faces identical constraints.

While adding a guard for defensive consistency (as done in isUrlSafeForSecrets() on lines 14–18) could be nice-to-have, it is not a contract break or behavioral regression for this application.

			> Likely an incorrect or invalid review comment.

Comment thread clients/agent-runtime/src/channels/mod.rs
Comment thread clients/agent-runtime/src/channels/telegram.rs
Comment thread clients/agent-runtime/src/skills/lockfile.rs
Comment thread clients/agent-runtime/src/skills/sandbox.rs
Comment thread clients/web/apps/dashboard/src/composables/useConfig.ts
Comment thread scripts/validate-docs-metadata.mjs
Comment thread scripts/validate-docs-metadata.mjs
- Refactor lockfile hash computation to avoid redundant file reads and clarify logic
- Update extract_user_text to prefer canonical content and handle empty parts correctly
- Add path fingerprinting for improved sandbox path error logging
- Refactor Telegram message parsing to consistently preserve caption text for non-image media and improve command extraction logic
- Harden useConfig conflict error handling for null/undefined payloads
- Generalize external link detection and restrict markdown link resolution to docs root in docs metadata validation
@yacosta738 yacosta738 merged commit ec60af0 into feat/258-expand-mcp-platform-support-beyond-tools-only-integration Mar 27, 2026
11 of 12 checks passed
@yacosta738 yacosta738 deleted the fix/sonarqube-code-smells branch March 27, 2026 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant