Skip to content

fix: resolve extension definitions from config#1671

Merged
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:fix_ext
Jun 9, 2026
Merged

fix: resolve extension definitions from config#1671
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:fix_ext

Conversation

@reddevillg

Copy link
Copy Markdown
Collaborator

If a matching definition is found, use its metadata directly. Fall back to manual construction only when no match exists.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors extension resolution in RunContext::resolve by matching extension layers against target layer definitions, and removes verbose logging in Cli::run. The review identifies a prefix matching bug and an unnecessary shouldEnable check in the matching logic. It suggests finding the best (longest or exact) matching definition directly, which would also allow the removal of the helper function isExtensionBelongTo.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread libs/linglong/src/linglong/runtime/run_context.cpp
Comment thread libs/linglong/src/linglong/runtime/run_context.cpp Outdated
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 14.28571% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libs/linglong/src/linglong/runtime/run_context.cpp 14.28% 10 Missing and 8 partials ⚠️
Files with missing lines Coverage Δ
libs/linglong/src/linglong/cli/cli.cpp 1.77% <ø> (+<0.01%) ⬆️
libs/linglong/src/linglong/runtime/run_context.cpp 15.38% <14.28%> (-0.13%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

If a matching definition is found, use its metadata directly. Fall back
to manual construction only when no match exists.
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

我来审查这段代码的改动,并从语法逻辑、代码质量、性能和安全性几个方面提出改进建议。

语法逻辑

  1. 删除了cli.cpp中的调试日志输出,这部分改动是合理的,因为调试信息通常在生产环境中不需要,并且可以通过日志级别控制来决定是否输出。

  2. 在run_context.cpp中新增了扩展定义查找逻辑,代码逻辑较为清晰,但有几个地方可以优化:

    • lambda函数matchedDef的逻辑有点复杂,可以考虑提取为单独的函数
    • 错误处理中使用了LINGLONG_ERR(manualDefs),这里应该使用LINGLONG_ERR(manualDefs.error())更合适

代码质量

  1. 命名和注释:

    • 变量matchedDef的命名可以更具描述性,比如matchingExtensionDef
    • 在复杂的逻辑块前应该添加注释说明其目的
  2. 代码结构:

    • 新增的扩展查找逻辑使函数变长,可以考虑将这部分逻辑提取到单独的函数中
    • makeManualExtensionDefine的调用应该检查其返回值是否为有效结果

性能

  1. 在扩展查找的循环中,每次都创建新的Extension实例(extension::ExtensionFactory::makeExtension(name)),这可能会影响性能。可以考虑:

    • 缓存已创建的扩展实例
    • 或者在循环外创建一次,然后复用
  2. 字符串操作:

    • def.name被重复赋值给name变量,可以直接使用def.name
    • 字符串拼接操作较多,可以考虑使用更高效的方式

安全性

  1. 在处理扩展引用时,应该进行更严格的输入验证:

    • 检查extLayer.getReference().id是否为空
    • 验证扩展名称是否符合安全规范
  2. 在使用targetInfo.extensions前,应该检查其是否为空,避免空指针解引用

具体改进建议

  1. 将扩展查找逻辑提取为单独函数:
static utils::error::Result<api::types::v1::ExtensionDefine> findMatchingExtension(
    const RuntimeLayer& extLayer,
    const RuntimeLayer& targetLayer) {
    // 实现查找逻辑
}
  1. 优化性能,减少不必要的对象创建:
// 可以在循环外创建扩展工厂实例
auto extFactory = extension::ExtensionFactory::getInstance();
for (const auto &def : *targetInfo.extensions) {
    if (extFactory->shouldEnable(def.name) && def.name == extLayer.getReference().id) {
        return &def;
    }
}
  1. 改进错误处理:
auto manualDefs = makeManualExtensionDefine({ extensionRefStr });
if (!manualDefs) {
    return LINGLONG_ERR("Failed to create manual extension define: {}", 
                       manualDefs.error().message());
}
  1. 添加输入验证:
if (extLayer.getReference().id.empty()) {
    return LINGLONG_ERR("Extension ID is empty");
}
if (!targetInfo.extensions) {
    return LINGLONG_ERR("Target layer has no extensions defined");
}
  1. 添加适当的日志记录:
LogI("Searching for extension {} in target layer {}", 
     extLayer.getReference().toString(),
     targetLayer->get().getReference().toString());

这些改进建议可以提高代码的可维护性、性能和安全性,同时保持原有功能的完整性。

@reddevillg

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors extension resolution in RunContext::resolve to dynamically match extension definitions from the target layer, falling back to manual definitions with a warning log if not found. It also removes redundant debug logging in cli.cpp and adds a warning log when getExtensionInfo fails in resolveLayer. The review feedback recommends adding a null check for the ext pointer returned by ExtensionFactory::makeExtension to prevent potential null pointer dereferences.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread libs/linglong/src/linglong/runtime/run_context.cpp
@reddevillg reddevillg requested review from ComixHe and dengbo11 June 9, 2026 03:54
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengbo11, reddevillg

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dengbo11 dengbo11 merged commit 6b93bf2 into OpenAtom-Linyaps:master Jun 9, 2026
16 of 17 checks passed
@reddevillg reddevillg deleted the fix_ext branch June 9, 2026 06:11
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.

3 participants