Skip to content

fix(cli): preserve dash options before subcommands - #4428

Merged
huangruiteng merged 1 commit into
loopx-project:mainfrom
Jim-jimu:codex/fix-dash-subcommand-options
Sep 15, 2026
Merged

huangruiteng merged 1 commit into
loopx-project:mainfrom
Jim-jimu:codex/fix-dash-subcommand-options

Conversation

@Jim-jimu

Copy link
Copy Markdown
Contributor

Summary

loopx dash --goal-id sample-a --port 19001 serve accepts the supplied options but silently replaces them with child-parser defaults, losing the Goal filter and requested port. loopx dash --goal-id sample-a generate also loses its Goal filter and can export other Goals.

Suppress defaults for duplicated child options so explicitly supplied parent values survive. Options supplied after the subcommand still take precedence, and no-option defaults stay unchanged. Add regression coverage for all five serve options, generate filtering, and real two-Goal registry exports.

Issue Or Task

Reproduced directly on main; no existing issue linked. Scope: session-dash CLI argument handling.

Validation

  • Tested revision: 9ed9e026dc80159e63127d524de4671766026604
  • Base revision: cd9cfa6d40b65ea0ea61f6b67ddae58167b845a6
  • Run state: finished
  • Input classes: synthetic, public_fixture
Check kind Result Evidence / limitation
regression_parity passed Same 16 dash tests: 8 fail / 8 pass with the upstream argument-registration function, 16 pass with the fix. Includes loss of the Goal filter through real registry reading and HTML generation.
unit passed python -m pytest -q tests/cli_commands/test_dash_options.py tests/test_cli_entrypoint.py tests/test_cli_argument_diagnostics.py: 131 passed, including the 16 dash tests.
real_entrypoint passed The three export cases use the actual entrypoint, a synthetic two-Goal registry and real rendering: unfiltered export contains both Goals; a filter before or after generate includes only the selected Goal.
integration passed python examples/session-dash-panel-smoke.py: existing projection, rendering and public-output boundary checks passed; some inputs and HTTP connections are test doubles.
static passed Ruff on both changed files; git diff --check upstream/main...HEAD.
integration not_run Full repository, Windows and installed-wheel suites; upstream PR CI pending PR creation.

Coverage: argument inheritance, explicit override precedence, defaults, dispatch and actual filtered export. The server-dispatch unit test substitutes the blocking server function. No model calls or live user data are used.

Frontend / Visual Evidence

  • UI impact: none
  • Before / After / States and viewports: N/A
  • Source data: none

Only CLI argument registration changes. Existing server, frontend and Lark code need no companion changes; their input fields and rendering contracts are unchanged.

Type of Change

  • Bug fix
  • Test update

LoopX Area

  • Public docs or presentation surface — session-dash CLI

Technical Direction

  • Core control-plane hardening
  • Target base branch: main
  • Direction tracker or promotion unit: N/A; bounded CLI bug fix.

Shared-authority RFC fixture impact

N/A. No authority, provider or state contract changes.

Boundary Checklist

  • Diff and PR text contain no private state, credentials, raw traces, internal links or local machine paths.
  • No maintainer-owned benchmark work is duplicated.
  • The change is limited to the reproduced argument-handling bug and its tests.
  • UI impact is marked none.
  • The commit includes a DCO Signed-off-by trailer.

The existing CLI module remains the sole owner; no new abstraction or further extraction is needed for this correction.

Signed-off-by: bmh201708 <49069997+bmh201708@users.noreply.github.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

动机

loopx dash 是两级解析:dash 组解析器声明了 --goal-id/--host/--port/--refresh-seconds/--verbose,serve/generate 子解析器又各声明了一遍同名参数并带上具体默认值。argparse 的子解析器会先在自己的新名字空间里求值,再回写父名字空间,于是"子解析器默认值"会覆盖"父解析器已被显式赋的值"。结果就是 loopx dash --goal-id sample-a --port 19001 serve 既不按 19001 监听、也不做 Goal 过滤;dash --goal-id sample-a generate 更严重——它会把整个 fleet 导出成"以为只导一个 Goal"的快照。我在 origin/main(c979cf11c)上直接解析确认:goal_id=None、port=8767、verbose=False,即父参数被静默丢弃。

改动思路

用 argparse 自己的语义修 argparse 的问题:给重复声明的子参数加 default=argparse.SUPPRESS,让子解析器在没收到该 flag 时不写属性,父值自然保留;子命令之后显式给出的 flag 仍然胜出。仓库里已有同类先例——loopx/cli_runtime.py:91 的 add_subcommand_format 用独立 dest(subcommand_format)避免了同名遮蔽,本 PR 是把同一原则用在 dash 的重复参数上,且没有引入新的合并层或参数搬迁。

具体改动

  1. loopx/cli_commands/dash.py:97,101-105:serve 的 --goal-id/--host/--port/--refresh-seconds/--verbose 默认值改为 argparse.SUPPRESS,并补一行注释说明"继承 dash 解析出的值,只有显式子参数才覆盖"。
  2. loopx/cli_commands/dash.py:112:generate --goal-id 同样改为 SUPPRESS,让 dash --goal-id X generate 真正窄化快照。
  3. 新增 tests/cli_commands/test_dash_options.py(151 行):五个 serve 选项的父位置生效、三种无参数场景默认值不变、父子同给时子值胜出、main() 分发到 serve_dash 的 kwargs、以及用真实两 Goal registry 跑出的 focus_goal_id/goal_count/HTML 过滤断言。

对主干的风险

  • 这是行为变更而不是纯修复:以前被静默忽略的父参数现在生效。依赖"传了也没用"的脚本会改变监听端口或导出范围;但对照探针显示无参数场景与"子命令后给 flag"的场景在 base/head 上完全一致,所以默认路径没有漂移。
  • 覆盖范围仅 dash 组。我按"同一文件里在 add_subparsers 之前与之后都声明同名选项"扫描了 loopx/cli_commands/*.py 与 loopx/cli.py,head 上没有第二处同类遮蔽,因此该修复在当前主干是完备的。
  • 残留脆弱点:这条规则靠注释与测试记录,没有机械约束;将来若有人再在父子两层重复声明同名参数并给具体默认值,同类静默丢失会重现。

验证(exact head 9ed9e026d,真实解析器/真实入口):

  • pytest tests/cli_commands/test_dash_options.py -q → 16 passed。
  • pytest tests/test_dashboard_command.py -q → 37 passed;python examples/session-dash-panel-smoke.py → OK。
  • base/head 对照探针(7 组 argv):['dash','--goal-id','sample-a','--port','19001','serve'] base None/8767 → head sample-a/19001;['dash','--verbose','serve'] base False → head True;['dash','generate']、['dash','serve']、['dash','generate','--goal-id','sample-a']、['dash','--goal-id','a','serve','--goal-id','b'] 两版本一致。

我的整体评价

根因定位准确、修法最小、回归测试覆盖到分发与真实 registry,符合"让下一次改动更容易定位"的方向。给出 APPROVE。

两点非阻塞说明:其一,该分支当前没有任何远端 CI 检查记录(GitHub 返回 "no checks reported on the 'codex/fix-dash-subcommand-options' branch"),所以本 head 的验证来自我在本地跑的上列三组命令,合并前建议先触发一次 CI 或确认由合并队列补跑;其二,若后续愿意再收一小步,可把"子解析器不得重复声明父参数默认值"做成解析器级断言或复用 cli_runtime 的既有 helper 家族,避免同类问题第三次出现。

English verdict: APPROVE at exact head 9ed9e02. The bug is real and reproduced on origin/main (parent-supplied --goal-id/--port/--verbose were silently replaced by child defaults), and the argparse.SUPPRESS fix restores them while keeping no-option defaults and post-subcommand overrides identical between base and head; validation is 16 passed on the new parser tests, 37 passed on tests/test_dashboard_command.py, the session dash panel smoke OK, and a seven-argv base/head probe. Two non-blocking notes: this branch reports no remote CI checks yet, and the SUPPRESS invariant is recorded by a comment plus tests rather than enforced mechanically.

@Jim-jimu

Copy link
Copy Markdown
Contributor Author

@huangruiteng 感谢 review!我确认了目前 4 个 Actions workflow 都处于 action_required,页面显示需要维护者批准运行。方便时麻烦批准一下,我们再根据 CI 结果处理后续问题。

@huangruiteng
huangruiteng merged commit e4932da into loopx-project:main Sep 15, 2026
@Jim-jimu
Jim-jimu deleted the codex/fix-dash-subcommand-options branch September 15, 2026 12:43
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.

2 participants