Skip to content

Conversation

@RomneyDa
Copy link
Collaborator

@RomneyDa RomneyDa commented Oct 21, 2025

Description

Fixes injected blocks from --agent being ignored when there is no other config source specified


Summary by cubic

Fixes the CLI so agent blocks passed via --agent are applied even when no other config is provided. Injected blocks now merge into the loaded assistant across all config load paths.

  • Bug Fixes
    • Handle “no-config” by unrolling and merging injected blocks instead of returning a placeholder.
    • Make unrollPackageIdentifiersAsConfigYaml return AssistantUnrolled directly; update merge calls.
    • Improve errors during unroll; throw clear failures when config is missing or fatal errors occur.
    • Update tests and ConfigService to use the new return type and access models via .models[0].

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

github-actions bot commented Oct 21, 2025

✅ Review Complete

Review of PR #8361: fix(cli): agent injected blocks ignored

Overall Assessment

The changes look good and effectively fix the issue where injected blocks from --agent were being ignored when no other config source was specified. The refactoring improves code clarity by having unrollPackageIdentifiersAsConfigYaml return AssistantUnrolled directly instead of wrapping it.

Specific Feedback

✅ Good Changes

  1. Line 198-203 (configLoader.ts): The "no-config" case now properly handles injected blocks instead of returning a placeholder. This is the core fix and looks correct.

  2. Lines 328, 388, 547 (configLoader.ts): Removing the conditional ?.block unwrapping simplifies the code now that the function returns the unwrapped type directly.

  3. Lines 420-428 (configLoader.ts): Better error handling with clearer error messages. The order check (errors before config) is correct.

⚠️ Potential Issues

Line 430 (configLoader.ts) - Type Safety Concern

return unrollResult.config;

The function now returns AssistantUnrolled but should handle the case where unrollResult.config might be undefined despite the check. Consider:

if (!unrollResult?.config) {
  throw new Error(`Failed to load config`);
}
return unrollResult.config; // TypeScript might still see this as potentially undefined

Recommendation: Add a non-null assertion or type guard if TypeScript complains, or ensure the unrollAssistantFromContent return type guarantees config is defined when there are no fatal errors.

Line 405 (configLoader.ts) - FILLER Name Changed
The name changed from "FILLER" to "Agent". While this is an improvement, verify that:

  • No other code depends on the "FILLER" name
  • "Agent" is an appropriate name for this use case
  • This doesn't conflict with any existing assistant names

Line 231 (ConfigService.ts) - Error Message Accuracy

throw new Error("Loaded default model contained no model block");

The error message says "no model block" but now we're checking models?.[0] on the unwrapped config. Consider updating to:

throw new Error("Loaded default model config contained no models");

📝 Testing

The test updates look correct - they properly reflect the new return type. However, consider adding a test specifically for the "no-config" path with injected blocks to ensure this bug doesn't regress.

Summary

The fix correctly addresses the issue. Main concerns are:

  1. Potential TypeScript type safety with the return value
  2. Verify the name change from "FILLER" to "Agent" doesn't have side effects
  3. Minor error message improvement suggestion

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

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Oct 21, 2025
@sestinj sestinj merged commit 6098fbf into main Oct 21, 2025
58 checks passed
@sestinj sestinj deleted the dallin/injected-blocks-bug branch October 21, 2025 04:20
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Oct 21, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 21, 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

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.

3 participants