Skip to content

Github support - #1222

Closed
muterC wants to merge 1 commit into
ZhuLinsen:mainfrom
muterC:zc260507_1
Closed

Github support#1222
muterC wants to merge 1 commit into
ZhuLinsen:mainfrom
muterC:zc260507_1

Conversation

@muterC

@muterC muterC commented May 7, 2026

Copy link
Copy Markdown

PR Type

  • fix
  • feat
  • refactor
  • docs
  • chore
  • test

Background And Problem

请描述当前问题、影响范围与触发场景。
(EN) Describe the problem, its impact, and what triggers it.

Scope Of Change

请列出本 PR 修改的模块和文件范围。
(EN) List the modules and files changed in this PR.

Issue Link

必须填写以下之一 / Fill in one of:

  • Fixes #<issue_number>
  • Refs #<issue_number>
  • 无 Issue 时说明原因与验收标准 / If no issue, explain the motivation and acceptance criteria

Verification Commands And Results

请填写你实际执行过的命令和关键结果(不要只写"已测试")。
(EN) Paste the commands you actually ran and their key output (don't just write "tested"):

# example
./scripts/ci_gate.sh
python -m pytest -m "not network"

关键输出/结论 / Key output & conclusion:

Compatibility And Risk

请说明兼容性影响、潜在风险(如无请写 None)。
(EN) Describe compatibility impact and potential risks (write None if not applicable).

  • 若本 PR 修改第三方模型 / API 的兼容语义、请求参数、路由前缀或 provider fallback,请提供官方来源链接或公告,并说明这是长期约束、当前运行时约束还是临时兼容处理。
    (EN) If this PR changes third-party model/API compatibility, request parameters, routing prefixes, or provider fallback behavior, include an official source link or announcement and clarify whether the rule is permanent, runtime-specific, or a temporary compatibility workaround.)
  • 若本 PR 依赖特定运行时 / 锁定依赖窗口(例如 LiteLLM 版本范围、OpenAI-compatible 路由、YAML alias 行为),请写明当前验证过的兼容范围与覆盖路径。
    (EN) If this PR depends on a specific runtime or pinned dependency window (for example a LiteLLM version range, OpenAI-compatible routing, or YAML alias behavior), state the compatibility window you verified and which code paths were covered.)
  • 若本 PR 触及运行时配置保存、清理、迁移或回填逻辑,请明确说明旧配置是否会被自动改写、清空、迁移或保持不变,以及用户如何恢复原行为。
    (EN) If this PR touches runtime config save/cleanup/migration/backfill logic, explicitly describe whether existing config is rewritten, cleared, migrated, or left intact, and how users can restore the previous behavior.)

Rollback Plan

请至少写一句可执行的回滚方案(必填)。
(EN) Provide at least one actionable rollback step (required).

  • 如果是兼容性修复,默认应写出最小回滚方式(例如 revert this PR),并说明是否需要额外回滚配置或数据迁移。
    (EN) For compatibility fixes, include the minimal rollback path (for example revert this PR) and whether any additional config or data rollback is required.)

EXTRACT_PROMPT Change (if applicable)

若本 PR 修改了 src/services/image_stock_extractor.py 中的 EXTRACT_PROMPT,请在此处粘贴完整变更后的 prompt。
If this PR changes EXTRACT_PROMPT in src/services/image_stock_extractor.py, paste the full updated prompt here:

展开 / Expand: Full EXTRACT_PROMPT
(paste full prompt here)

Checklist

  • 本 PR 有明确动机和业务价值 / This PR has a clear motivation and value
  • 已提供可复现的验证命令与结果 / Reproducible verification commands and results are included
  • 已评估兼容性与风险 / Compatibility and risk have been assessed
  • 已提供回滚方案 / A rollback plan is provided
  • 若涉及用户可见变更,已同步更新相关文档与 docs/CHANGELOG.mdREADME.md 仅在首页级信息变化时更新,细节优先写入 docs/*.md / If user-visible changes are included, relevant docs and docs/CHANGELOG.md are updated; README.md is updated only for homepage-level changes, with details kept in docs/*.md

@muterC
muterC requested a review from ZhuLinsen as a code owner May 7, 2026 13:45
@github-actions github-actions Bot added ai ci/cd configuration documentation Improvements or additions to documentation size/M testing labels May 7, 2026
@github-actions

github-actions Bot commented May 7, 2026

Copy link
Copy Markdown

🤖 自动审查报告

项目 结果
📊 变更文件 10 个
➕ 新增行数 153 行
➖ 删除行数 5 行
🔍 静态检查 ✅ 通过
🧠 AI 审查 ✅ 已完成

📁 修改的文件

  • 📝 .env.example (+17/-0)
  • 📝 .github/workflows/daily_analysis.yml (+11/-0)
  • 📝 apps/dsa-web/src/components/settings/LLMChannelEditor.tsx (+9/-2)
  • 📝 apps/dsa-web/src/components/settings/__tests__/llmProviderTemplates.test.ts (+2/-0)
  • 📝 apps/dsa-web/src/components/settings/llmProviderTemplates.ts (+23/-1)
  • 📝 docs/CHANGELOG.md (+2/-0)
  • 📝 docs/llm-providers.md (+2/-0)
  • 📝 src/config.py (+78/-1)
  • 🆕 test_out.txt (+0/-0)
  • 📝 tests/test_daily_analysis_workflow_llm_env.py (+9/-1)

🧠 AI 代码审查意见

结论

Not Ready

结构化审查结果

必要性

通过

  • 本 PR 引入了对 GitHub Copilot 作为 LLM 渠道的支持,包括 OAuth 设备流和兼容 OpenAI 接口两种模式,并提供了零配置默认启用机制。这为用户提供了更多的 LLM 选择,具有明确的业务价值。

关联性

不通过

  • PR 描述中未提供关联 Issue 链接 (Fixes #<issue_number>Refs #<issue_number>)。
  • 在无 Issue 的情况下,也未说明 PR 的动机和验收标准。这违反了审查规则中的“关联性”要求。

类型

建议类型: feat

  • 根据代码变更内容,本 PR 新增了 GitHub Copilot LLM 渠道支持及零配置默认启用,属于新功能开发。

描述完整性

不完整

  • 缺失项:
    • PR Type: 未勾选。
    • Background And Problem: 未填写。
    • Scope Of Change: 未填写。
    • Issue Link: 未填写,且未说明无 Issue 的原因和验收标准。
    • Verification Commands And Results: 未填写实际执行的命令和关键结果。虽然 CI 通过了静态检查,但功能性验证的证据缺失,无法确认验证是否充分。
    • Compatibility And Risk: 未填写。
    • Rollback Plan: 未填写,这是阻断合入的必填项。
    • Checklist: 所有条目均未勾选。

风险级别

  • 关键风险:
    1. 验证不足: 缺少功能验证命令和结果,无法确认 GitHub Copilot 渠道在实际运行时(尤其是在 OAuth 设备流场景下)的稳定性、兼容性以及错误处理是否充分。
    2. 配置默认行为: src/config.py 中新增的“零配置默认启用 GitHub Copilot 渠道”可能会在用户预期之外触发 OAuth 流程。虽然有日志提示和禁用环境变量 (LLM_DEFAULT_COPILOT=false),但如果用户不阅读日志,可能会感到困惑。
    3. CI 环境限制: daily_analysis.yml 中的注释明确指出 OAuth 设备流不适用于无人值守 CI。这意味着需要在本地环境进行充分测试,而这部分的验证证据是缺失的。

必改项

  1. 补充回滚方案: 在 PR 描述中补充明确可执行的回滚方案,例如 revert this PR
  2. 补充验证命令与结果: 详细说明在本地如何验证 GitHub Copilot 渠道的连通性、OAuth 流程、模型调用等,并附上关键输出。这是确保新功能质量的核心证据。
  3. 补充 Issue 链接或无 Issue 理由: 填写关联 Issue,或说明无 Issue 的原因并提供明确的验收标准。
  4. 勾选 PR Type: 将 PR Type 勾选为 feat
  5. 补全 tests/test_daily_analysis_workflow_llm_env.py 文件内容:
    • 根据提供的 diff,文件 tests/test_daily_analysis_workflow_llm_env.py 的 diff 内容在 if 语句处被截断,if 条件不完整。请补齐完整变更内容。

建议项

  1. 填写 Background And Problem / Scope Of Change: 补充 PR 的背景、解决的问题以及涉及的模块和文件范围,这将使 PR 描述更完整、易于理解。
  2. 补充 Compatibility And Risk: 详细说明引入 GitHub Copilot 渠道可能带来的兼容性影响和潜在风险(例如 OAuth 流程对用户交互的依赖,或与现有 LiteLLM 版本的兼容性),即使是 None 也应明确写出。
  3. 运行 ./scripts/ci_gate.sh 并说明结果: 由于本 PR 涉及 Python 后端改动,且 ./scripts/ci_gate.sh 不在 CI 覆盖范围,建议在 PR 描述中说明是否已在本地执行该命令,并贴出关键结果或说明跳过原因。
  4. 勾选 Checklist: 在完成所有审查项后,请勾选 PR 描述中的 Checklist 以表明已完成自检。
  5. 考虑默认启用 Copilot 的用户引导: 对于 main.py / server.py 默认启用 GitHub Copilot 渠道的行为,可以在文档中更显著地突出 LLM_DEFAULT_COPILOT=false 的禁用方式,或考虑在首次运行时提供更友好的交互提示,确保用户理解其行为。

💡 提示: 请确保代码已通过本地测试,并遵循项目代码规范。

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 038d9a4f71

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

{ value: 'anthropic', label: 'Anthropic' },
{ value: 'vertex_ai', label: 'Vertex AI' },
{ value: 'ollama', label: 'Ollama' },
{ value: 'github_copilot', label: 'GitHub Copilot (OAuth)' },

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve Copilot protocol when loading channel settings

Adding github_copilot to the selectable protocols here introduces a regression because the editor’s protocol parser (normalizeProtocol used by inferProtocol) still falls back unknown values to openai. In practice, when an existing config contains LLM_COPILOT_PROTOCOL=github_copilot, opening and saving the Settings page rewrites that channel as openai, which breaks the intended OAuth device-flow path and can make the channel fail without a manually provided token/key. Please add explicit github_copilot normalization (and related alias handling) so Copilot channels round-trip safely.

Useful? React with 👍 / 👎.

@ZhuLinsen ZhuLinsen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

评审结论

  • 必要性:通过。新增 GitHub Copilot 作为 LLM 渠道有实际使用价值,但默认启用策略需要先收敛运行时风险。
  • 是否有对应 issue:无。未检测到 Fixes/Closes/Refs,Issue Link 仍未有效填写。
  • PR 类型:feat。新增后端 LLM 渠道、Web 预设、Actions/env 映射和相关文档。
  • description 完整性:不完整。缺少 PR Type 勾选、背景/范围、Issue 或验收标准、实际验证结果、兼容性风险说明和回滚方案。
  • 是否可直接合入:不可。CI 当前成功且 mergeable_state=blocked 本身不作为阻断,但下面的正确性与兼容性问题需要先处理。

主要问题

  1. [Correctness blocker] apps/dsa-web/src/components/settings/LLMChannelEditor.tsx:diff 只把 github_copilot 加到可选协议和“无需 Key”提示,但协议推断/归一化路径未同步接纳该值,已展开快照中的 KNOWN_MODEL_PREFIXES 仍不包含 github_copilot。风险是已保存或加载的 Copilot 渠道被回退成 openai,再次保存后丢失 LiteLLM 原生 OAuth provider 语义。需要补齐解析逻辑,并加 Web 回归测试覆盖加载后保存仍保持 github_copilot
  2. [Compatibility blocker] src/config.pyLLM_DEFAULT_COPILOT 默认 true 会在完全未配置 LLM 时自动启用 Copilot。LiteLLM 官方文档说明首次使用 github_copilot/ 会触发 OAuth device flow:https://docs.litellm.ai/docs/providers/github_copilot 。这会让 main.pyserver.py、Docker、GitHub Actions 等无交互环境可能阻塞或失败,且依赖付费 Copilot 订阅。建议改为 opt-in,或补非交互保护、用户可见提示和回归测试。
  3. [Compatibility blocker] apps/dsa-web/src/components/settings/llmProviderTemplates.tsdocs/llm-providers.mdtests/test_daily_analysis_workflow_llm_env.py:新增 Copilot 示例包含 claude-sonnet-4,但 GitHub 官方 Copilot 模型文档已列出 Claude Sonnet 4 于 2026-05-01 退休,建议替代为 Claude Sonnet 4.6:https://docs.github.com/en/copilot/reference/ai-models/supported-models 。当前日期为 2026-05-07,该示例会误导用户配置已过期模型。
  4. [Process blocker] PR 描述仍基本是模板正文:没有 issue 或无 issue 验收标准,没有实际验证命令与关键输出,未说明 Copilot/LiteLLM 的官方来源、运行时兼容范围、旧配置影响和恢复方式,也没有最小回滚方案。

🤖 此回复由 OpenReview Bot 自动生成,仅供参考。如有疑问请 @维护者。

@ZhuLinsen ZhuLinsen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

跟进结论

  • 结论:接受,当前 backend-gate 确实失败,且失败直接落在本 PR 新增的默认 Copilot 配置行为上。
  • 是否仍有阻断:有,离线测试失败属于阻断项,不能仅靠 mergeable_state=blocked 或 web-gate 通过来放行。

说明

  1. backend-gate 中语法、flake8、deterministic checks 已通过,失败发生在 python -m pytest -m "not network";定位不需要等待 CI,失败点已经明确。
  2. src/config.py 新增的 LLM_DEFAULT_COPILOT 默认 true 会在无任何 LLM 配置时自动生成 copilot 渠道。CI 中 tests/test_llm_channel_config.py 有 3 个用例原本期望禁用/空配置后 llm_channelsllm_model_list 为空,现在实际得到 github_copilot/gpt-4o,说明该默认行为改变了既有配置语义。
  3. 同一默认行为还触发了真实 LiteLLM Copilot OAuth 路径,tests/test_pipeline_realtime_indicators.pytests/test_report_schema.py 多个离线用例报 Failed to get access token after 3 attempts,模型为 github_copilot/gpt-4o。这验证了之前关于无人值守/离线环境风险的判断,不只是文档描述问题。
  4. 建议优先把 Copilot 默认改为 opt-in,或至少在 CI/测试/无交互环境默认关闭,并补回归测试证明禁用渠道、空配置、离线测试不会自动进入 OAuth device flow。LiteLLM 文档也说明 github_copilot/ 首次使用会走 OAuth device flow:https://docs.litellm.ai/docs/providers/github_copilot
  5. apps/dsa-web/src/components/settings/LLMChannelEditor.tsx 仍未在 normalizeProtocol / alias 路径中接纳 github_copilot,Web 设置页保存后仍有把 Copilot 协议回退为 openai 的风险;web-gate 通过不能覆盖这个 round-trip 场景。
  6. claude-sonnet-4 示例仍在 .env.exampledocs/llm-providers.mdllmProviderTemplates.ts 中出现。GitHub 官方模型页显示 Claude Sonnet 4 已于 2026-05-01 退休,建议替代 Claude Sonnet 4.6:https://docs.github.com/en/copilot/reference/ai-models/supported-models

🤖 此回复由 OpenReview Bot 自动生成,仅供参考。如有疑问请 @维护者。

@ZhuLinsen

Copy link
Copy Markdown
Owner

感谢贡献,这个方向本身有价值,但当前 PR 暂不适合继续合入,我先关闭处理。

主要原因:

  1. 当前实现把 GitHub Copilot 作为无 LLM 配置时的默认渠道,会改变项目原有“空配置即不启用 LLM”的语义,并且在 CI / 离线 / 无交互环境下触发 LiteLLM Copilot OAuth device flow,已经导致 backend-gate 的离线测试失败。
  2. Web 设置页对 github_copilot 的协议 round-trip 还不完整,存在加载后保存时被回退成 openai 的风险,会破坏原生 OAuth provider 语义。
  3. 示例模型里仍包含已不适合继续推荐的 claude-sonnet-4,容易误导用户配置。
  4. PR 描述、验证命令、兼容性风险、回滚方案和验收标准都没有补齐,不满足当前仓库的合入要求。

@ZhuLinsen ZhuLinsen closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants