Skip to content

Conversation

@RomneyDa
Copy link
Collaborator

@RomneyDa RomneyDa commented Oct 20, 2025

Description

  • Add error handling to default model injection
  • Check if saved config URI exists - skip if doesn't
  • Allow local config usage for headless
  • Logic tweaks

Summary by cubic

Restores config fallback in headless CLI so runs can proceed with local/default config. Adds safe handling for missing saved URIs and default model injection to prevent crashes.

  • Bug Fixes
    • Allow local config usage in headless when no saved config is available.
    • Verify saved config URI exists (supports file: URIs) and warn if missing.
    • Add error handling for default model injection; throw a clear error in headless when no model can be loaded.
    • Pass headless state into ConfigService to apply mode-specific behavior.
    • Prefer persisted model name when available in ModelService.

@RomneyDa RomneyDa requested a review from a team as a code owner October 20, 2025 21:39
@RomneyDa RomneyDa requested review from Patrick-Erichsen and removed request for a team October 20, 2025 21:39
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 20, 2025
@github-actions
Copy link

github-actions bot commented Oct 20, 2025

✅ Review Complete

Code Review for PR #8356

Overall Assessment

This PR addresses a critical issue with config fallback in headless CLI mode. The changes are well-structured and improve error handling. Here are some specific observations:


✅ Good Changes

  1. Restored fallback logic - Moving the headless check after saved-uri validation allows local configs to work properly
  2. File existence validation - Checking if saved URI exists before using it prevents crashes
  3. Better error messaging - Differentiating headless vs non-headless error messages improves debugging

🔍 Issues & Suggestions

1. Missing import validation (extensions/cli/src/configLoader.ts:16)

import { fileURLToPath } from "node:url";

The fileURLToPath import is added but only used for file: URIs. Consider adding error handling for malformed URIs:

if (savedUri.startsWith("file:")) {
  try {
    const filePath = fileURLToPath(savedUri);
    if (fs.existsSync(filePath)) {
      return { type: "saved-uri", uri: savedUri };
    }
  } catch (err) {
    logger.warn(`Invalid file URI: ${savedUri}`);
  }
} else if (fs.existsSync(savedUri)) {
  return { type: "saved-uri", uri: savedUri };
}

2. Typo in warning message (extensions/cli/src/configLoader.ts:112)

logger.warn("Saved URI does not exists");

Should be: "Saved URI does not exist"

3. Silent error swallowing (extensions/cli/src/services/ConfigService.ts:236-245)

When not in headless mode, errors are logged but execution continues without a model. This could lead to confusing downstream failures. Consider:

  • Re-throwing after logging, OR
  • Setting a flag that subsequent code can check
} else {
  logger.error("Failed to load default model with no model specified", e);
  throw new Error("No model specified and failed to load default model");
}

4. Logic clarity (extensions/cli/src/services/ModelService.ts:67-71)

The variable rename (persistedName) is good, but the logic is now redundant:

const persistedName = getModelName(authConfig);
if (persistedName) {
  preferredModelName = persistedName;  // Just assigns the same value
  modelSource = "persisted";
}

Could simplify to:

preferredModelName = getModelName(authConfig);
if (preferredModelName) {
  modelSource = "persisted";
}

🧪 Testing Considerations

No test changes included. Consider adding tests for:

  1. Headless mode with missing saved URI
  2. File URI validation with malformed URIs
  3. Default model injection failure in headless vs non-headless

Security & Performance

  • ✅ No security vulnerabilities introduced
  • ✅ File system checks are synchronous but acceptable for CLI config loading
  • ✅ No breaking changes to API surface

Recommendation: Approve with minor fixes (typo + error handling improvement)


@RomneyDa RomneyDa added the hotfix Should be considered as a hotfix for main releases label Oct 20, 2025
@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Oct 20, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 20, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 3 files

Prompt for AI agents (all 2 issues)

Understand the root cause of the following 2 issues and fix them.


<file name="extensions/cli/src/configLoader.ts">

<violation number="1" location="extensions/cli/src/configLoader.ts:106">
Checking saved config existence this way drops any persisted slug:// config URI, so we no longer honor saved assistants when a slug was stored.</violation>

<violation number="2" location="extensions/cli/src/configLoader.ts:107">
Guard fileURLToPath against malformed file: URIs; the call can throw and crash during existsSync. Wrap the conversion in try/catch and skip/log invalid savedUri values instead of letting an exception propagate.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@RomneyDa RomneyDa merged commit ddf4e0f into main Oct 20, 2025
54 of 56 checks passed
@RomneyDa RomneyDa deleted the dallin/cli-loading-fixes branch October 20, 2025 23:46
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Oct 20, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2025
@sestinj
Copy link
Contributor

sestinj commented Oct 21, 2025

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sestinj
Copy link
Contributor

sestinj commented Oct 22, 2025

🎉 This PR is included in version 1.27.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sestinj
Copy link
Contributor

sestinj commented Oct 22, 2025

🎉 This PR is included in version 1.30.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sestinj
Copy link
Contributor

sestinj commented Oct 29, 2025

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

hotfix Should be considered as a hotfix for main releases lgtm This PR has been approved by a maintainer released size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants