feat: add global --debug flag for diagnostic output#488
Closed
heyumeng154-alt wants to merge 18 commits intolarksuite:mainfrom
Closed
feat: add global --debug flag for diagnostic output#488heyumeng154-alt wants to merge 18 commits intolarksuite:mainfrom
heyumeng154-alt wants to merge 18 commits intolarksuite:mainfrom
Conversation
|
|
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a global Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (os.Args)
participant FlagSet as FlagSet (pflag)
participant Root as Root Execute
participant Factory as Factory
participant ErrOut as IOStreams.ErrOut
Client->>FlagSet: provide args (--debug ...)
Root->>FlagSet: RegisterGlobalFlags(&globals)
FlagSet-->>Root: parse -> globals.Debug
Root->>Factory: create Factory
Root->>Factory: set DebugEnabled = globals.Debug
Factory->>ErrOut: Debugf("[DEBUG] ...") (only if DebugEnabled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-15-debug-flag-design.md`:
- Around line 63-73: Add a language tag to the fenced code block containing the
debug flow so it satisfies markdownlint MD040; change the opening triple
backticks to include a language (e.g., ```text) for the block that lists the
flow from "用户运行：lark-cli --debug..." through the steps referencing
GlobalOptions.Debug, cmd/root.go, f.DebugEnabled and f.Debugf("message"),
leaving the content unchanged otherwise.
In `@docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md`:
- Line 88: Replace the typo "断assert" with the correct Chinese term "断言" in the
negative-case checklist; locate the string "断assert" (exact text) in the
document and update it to "断言" so terminology is consistent across the spec.
In `@internal/core/config.go`:
- Around line 92-97: FindApp currently treats the literal "default" as an alias
and calls CurrentAppConfig(""), which can recurse when CurrentApp == "default"
and breaks callers that expect literal lookup; revert FindApp to only perform
literal name/appId matching and add a new method ResolveProfileSelector(name
string) on MultiAppConfig that returns CurrentAppConfig("") when name ==
"default" otherwise calls FindApp(name); update CLI/selector callers (e.g.,
cmd/profile/add.go and cmd/profile/use.go) to call ResolveProfileSelector for
alias semantics while leaving other code using FindApp unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a36bc52-0c15-48b5-a530-d46be70724fe
📒 Files selected for processing (9)
cmd/global_flags.gocmd/global_flags_test.gocmd/root.godocs/superpowers/plans/2026-04-15-debug-flag.mddocs/superpowers/specs/2026-04-15-debug-flag-design.mddocs/superpowers/specs/2026-04-15-debug-flag-test-plan.mdinternal/cmdutil/factory.gointernal/cmdutil/factory_debug_test.gointernal/core/config.go
Change-Id: I3c9f0847cddf8fa9bfe5822f763ec6302e2f062a
Change-Id: I73d6aef8feb385b4a14da24fc44fd6e920b700cd
Add a Debug bool field to the GlobalOptions struct and register a --debug boolean flag in RegisterGlobalFlags to enable debug logging support. Change-Id: I83556abb42b289996aad3d4b333c46c4c81f4bf5
Added DebugEnabled bool field to Factory struct to track debug mode state. Implemented Debugf method to write debug output to stderr with [DEBUG] prefix when debug mode is enabled, following the same pattern as other Factory methods. Change-Id: Ic8200fbfd8b6a42be5280c36920dec158834c8d8 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Change-Id: I4c5f10bb2b61bf76a6b1e40c372862bd51bc597a
This modification parses the --debug flag early in Execute() and passes the Debug value from GlobalOptions to Factory.DebugEnabled, enabling the Debugf() method to output debug messages when the flag is enabled. The parsing uses a temporary pflag.FlagSet to extract the debug value without interfering with the normal Cobra command tree processing. Change-Id: I6f2d16870b8ec0048dae32874c34b53b9a08c8bb
Change-Id: I098d4abbb95fc6091130d537ced86fde80a79d46
Change-Id: I737e12dd01abc6d445a800af27cfbc3731d2c3d4
Add special case handling in FindApp() to resolve --profile default to the currently active app configuration. This allows tests and users to reference the default profile by name without knowing the actual profile name. Fixes failing E2E tests that expect --profile default to work. Change-Id: I4c197873ac99f5bbc251b4bb60424364810537d9
…ommand-arg order Change-Id: Ib2804573074c48af480734912107b46b60e05398
heyumeng154-alt
force-pushed
the
feat/add-debug-flag
branch
from
e947c5d to
bb7b31f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md (1)
88-88:⚠️ Potential issue | 🟡 MinorFix wording typo (
断assert→断言).Line 88 still has mixed wording and should be normalized for consistency.
✏️ Suggested edit
-- 断assert： +- 断言：🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md` at line 88, Replace the mixed-language typo "断assert" with the correct Chinese term "断言" in the document; locate the exact string "断assert" in the spec text and update it to "断言" so the wording is consistent throughout the file.docs/superpowers/specs/2026-04-15-debug-flag-design.md (1)
63-73:⚠️ Potential issue | 🟡 MinorAdd a language tag to this fenced block (MD040).
Please specify a language (e.g.,
text) on the opening fence.🧩 Suggested edit
-``` +```text 1. 用户运行：lark-cli --debug +calendar agenda ↓ 2. Cobra 解析 --debug 标志到 GlobalOptions.Debug = true ↓ 3. cmd/root.go 创建 Factory，设置 f.DebugEnabled = opts.Debug ↓ 4. 命令执行时可调用 f.Debugf("message") ↓ 5. 如果 DebugEnabled 为 true，消息输出到 stderr；否则不输出</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/superpowers/specs/2026-04-15-debug-flag-design.mdaround lines 63 - 73,
The fenced code block showing the debug-flag flow lacks a language tag (MD040);
update the opening fence for that block (the triple-backtick that precedes the
numbered steps) to include a language label such as text (e.g., change ``` tofence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-15-debug-flag-design.md`:
- Line 68: The code currently sets Factory.DebugEnabled = opts.Debug but the
implementation flow and bootstrap wiring use the global variable named
globals.Debug; update the assignment in cmd/root.go where you create the Factory
(the Factory instance and its DebugEnabled field) to use globals.Debug instead
of opts.Debug so the wiring matches the rest of the codebase.
In `@docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md`:
- Around line 77-84: The test case claiming `lark-cli +calendar --debug agenda`
should not enable global debug conflicts with the bootstrap global-flag parsing;
update the spec so the negative case is removed or rewritten to assert that
`--debug` is recognized as a global flag regardless of position (since the
bootstrap parses global flags from argv before command execution). Edit the
entry containing the example `lark-cli +calendar --debug agenda` and mention the
bootstrap/global-flag parsing behavior (referencing "bootstrap" and the
`--debug` flag) so the test expectations match current implementation.
---
Duplicate comments:
In `@docs/superpowers/specs/2026-04-15-debug-flag-design.md`:
- Around line 63-73: The fenced code block showing the debug-flag flow lacks a
language tag (MD040); update the opening fence for that block (the
triple-backtick that precedes the numbered steps) to include a language label
such as text (e.g., change ``` to ```text) so the markdown linter passes; leave
the block contents and closing fence unchanged.
In `@docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md`:
- Line 88: Replace the mixed-language typo "断assert" with the correct Chinese
term "断言" in the document; locate the exact string "断assert" in the spec text
and update it to "断言" so the wording is consistent throughout the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50f5aff5-a1d6-416d-b4f2-e8edecf6bf6e
📒 Files selected for processing (9)
cmd/global_flags.gocmd/global_flags_test.gocmd/root.godocs/superpowers/plans/2026-04-15-debug-flag.mddocs/superpowers/specs/2026-04-15-debug-flag-design.mddocs/superpowers/specs/2026-04-15-debug-flag-test-plan.mdinternal/cmdutil/factory.gointernal/cmdutil/factory_debug_test.gointernal/core/config.go
✅ Files skipped from review due to trivial changes (4)
- cmd/root.go
- cmd/global_flags_test.go
- internal/cmdutil/factory_debug_test.go
- docs/superpowers/plans/2026-04-15-debug-flag.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/config.go
- cmd/global_flags.go
Change-Id: I66e14b7e01c650dabd604fb9067ff458363f089f
Author
|
✅ Fixed in commit f708079: Prevented infinite recursion in FindApp by checking if |
Author
|
🔍 Review of remaining CodeRabbit comments:
All critical code issues have been resolved. The feature is complete and tested. |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b0767e5d952c321a077bf53d77f1b13ca762f3f3🧩 Skill updatenpx skills add heyumeng154-alt/cli#feat/add-debug-flag -y -g |
Adds tests_e2e/cmd/ with three test suites covering the global --debug flag: workflow (10 cases), consistency, and integration with other flags. Also adds the e2e-tester acceptance report (10/10 scenarios passed). Change-Id: Ied3235acee4cdf1a572fc4d96821361d639cf338 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add 'text' language tag to fenced code block (MD040) - Fix 'globals.Debug' variable name in data flow diagram - Fix typo '断assert' → '断言' in test plan Change-Id: Iddf7a10a77c95774370c2b4dcc7a0ee5c14d5508 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests_e2e/cmd/2026_04_15_debug_flag_test.go (1)
119-130: Test case lacks meaningful assertions.The
debug_placement_after_commandtest runs the command but discards the result without asserting any expected behavior. The comment claims exit code is "checked implicitly," butrequire.NoError(t, err)only verifiesRunCmddidn't error—it doesn't validate the command's exit code or output.Consider explicitly asserting the expected behavior:
♻️ Proposed fix: Add meaningful assertions
t.Run("debug_placement_after_command", func(t *testing.T) { // Test --debug placed after command (not as global flag) - // This tests that --debug is position-sensitive + // When --debug appears after 'api', it's passed to the subcommand, not parsed globally. + // Per SetInterspersed(true) behavior, the api command may accept it. result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"api", "--debug", "GET", "/open-apis/contact/v3/users"}, }) require.NoError(t, err) - // Exit code could be 0 or non-zero depending on if --debug is accepted by api command - // The important thing is that it behaves differently than global --debug - // If it fails, that's correct behavior; if it passes, --debug was passed to api subcommand - _ = result // result used to verify command executes (exit code checked implicitly) + // Document actual behavior: either the command succeeds (api accepts --debug) + // or fails (api doesn't recognize --debug). Assert whichever is expected. + // For now, just verify it doesn't panic and capture exit code. + t.Logf("exit code when --debug after command: %d", result.ExitCode) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_e2e/cmd/2026_04_15_debug_flag_test.go` around lines 119 - 130, The test function debug_placement_after_command currently calls clie2e.RunCmd and discards result; update it to assert concrete expectations by inspecting the returned result (the variable result from clie2e.RunCmd) and err: check result.ExitCode (use require.Equal or require.NotEqual as appropriate for the intended behavior), and assert on result.Stdout/StdErr content to verify whether debug logging appears when --debug is placed after the subcommand (e.g., require.Contains/NotContains); keep require.NoError for RunCmd but add explicit assertions against result to make the test meaningful.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@E2E_ACCEPTANCE_REPORT_2026_04_15.md`:
- Line 7: Remove the hardcoded developer-local path string
"/Users/bytedance/work/cli-heyumeng154-alt" from the report and replace it with
a neutral, portable reference such as a relative path, the repository name, or a
placeholder like "<project-root>" so the file is not exposing personal
filesystem information and remains useful to other readers; update any
accompanying sentence that relies on that exact path to use the new placeholder
or relative path instead.
In `@tests_e2e/cmd/2026_04_15_debug_flag_test.go`:
- Around line 218-241: The tests "debug_with_format_json" and
"debug_format_order" pass the local flag "--format" before the "api" subcommand
which fails because "--format" is registered on the api command (cmd.Flags())
not globally; fix by either moving "--format" into the Args slice after the
"api" subcommand and its positional args (e.g. put "--format", "json" after
"/open-apis/...") in both clie2e.RunCmd calls, or if you intend "--format" to be
global, register it as a persistent flag on the root command instead of
cmd.Flags(); update the Args in the tests or change the flag registration
accordingly.
---
Nitpick comments:
In `@tests_e2e/cmd/2026_04_15_debug_flag_test.go`:
- Around line 119-130: The test function debug_placement_after_command currently
calls clie2e.RunCmd and discards result; update it to assert concrete
expectations by inspecting the returned result (the variable result from
clie2e.RunCmd) and err: check result.ExitCode (use require.Equal or
require.NotEqual as appropriate for the intended behavior), and assert on
result.Stdout/StdErr content to verify whether debug logging appears when
--debug is placed after the subcommand (e.g., require.Contains/NotContains);
keep require.NoError for RunCmd but add explicit assertions against result to
make the test meaningful.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 004b2cce-a287-4a49-b892-a22f6c56d86e
📒 Files selected for processing (3)
E2E_ACCEPTANCE_REPORT_2026_04_15.mdtests_e2e/cmd/2026_04_15_debug_flag_test.gotests_e2e/cmd/coverage.md
✅ Files skipped from review due to trivial changes (1)
- tests_e2e/cmd/coverage.md
Change-Id: I88ef0be4659b9e26d9fae2bcf8a45db716d686b9 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-15-debug-flag-design.md`:
- Line 112: The example uses grep [DEBUG] which will be treated as a regex
character class; update the doc text to show a literal match by either escaping
the brackets (e.g., '\[DEBUG\]') or by recommending fixed-string matching with
grep -F '[DEBUG]'; change the line containing "grep [DEBUG]" to one of these
literal-safe forms so the example matches the literal token "[DEBUG]".
- Around line 49-57: The example Debugf implementation is unsafe because it
dereferences f.IOStreams.ErrOut without nil checks; update the snippet for
Factory.Debugf to follow the project's nil-safe guard pattern by first verifying
f.DebugEnabled and that f.IOStreams and f.IOStreams.ErrOut are non-nil before
writing, and then write the formatted debug message to ErrOut (prefixed with
"[DEBUG]") so it will not panic when IOStreams or ErrOut are nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c16814c7-72c0-4f11-9d21-0cfdfdca8b84
📒 Files selected for processing (2)
docs/superpowers/specs/2026-04-15-debug-flag-design.mddocs/superpowers/specs/2026-04-15-debug-flag-test-plan.md
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/specs/2026-04-15-debug-flag-test-plan.md
- Update Debugf snippet to match nil-safe implementation - Fix 'grep [DEBUG]' → 'grep -F "[DEBUG]"' to avoid regex misinterpretation Change-Id: I4ed1020b5a40c74adca8a80005462f160178ea36 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The test plan previously claimed `lark-cli +calendar --debug agenda` should not enable global debug. This contradicted actual behavior: bootstrap parses global flags from argv before command execution, and SetInterspersed(true) makes --debug position-independent. Aligns spec with observed behavior (per E2E acceptance report 发现1). Change-Id: Ib30d608c224a074da71a242dceb37b201a1bfaa8 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
heyumeng154-alt
changed the title
Collaborator
|
1 |
--format is a local flag on the api subcommand, not a global flag. Placing it before the subcommand violates Cobra's flag parsing rules and causes failures in CI coverage where the binary is resolved. Change-Id: Ie1bde4b6f13f5c0dfae6bfffa6b45e743148afb5 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Addressed the remaining review feedback:
All other CodeRabbit comments (markdown language tags, typo fix, Debugf example, grep usage, hardcoded path) were addressed in earlier commits. Ready for another look when you have time. Thanks! |
MaxHuang22
reviewed
Apr 16, 2026
| return &m.Apps[0] | ||
| } | ||
| return nil | ||
| } |
Collaborator
There was a problem hiding this comment.
FindApp("default") 破坏了函数语义，建议改到 CurrentAppConfig() 层处理。
FindApp() 的职责是按名称或 appId 做精确查找，被多处调用：
cmd/profile/add.go用它做唯一性校验cmd/profile/use.go用它做直接查找
在这里注入 "default" 别名逻辑后，若某用户真的创建了一个名为 "default" 的 profile，行为将不符合预期。
建议：将别名逻辑移到 CurrentAppConfig(profileOverride string) 内部处理——在 profileOverride == "default" 时直接返回当前活跃 app，而不是依赖 FindApp 的副作用。
另外，这个改动和本 PR 的主题（--debug flag）不相关，建议拆分到独立 PR，方便单独审查和回滚。
Author
There was a problem hiding this comment.
已在 c18376d 中移除 FindApp("default") 别名逻辑，恢复 FindApp 的精确查找语义。这部分改动会拆到独立 PR 中，按建议在 CurrentAppConfig() 层处理。感谢 review 🙏
Reviewers (MaxHuang22, liuxinyanglxy) pointed out that injecting a "default" alias inside FindApp() breaks its precise-lookup semantics and is unrelated to the --debug flag feature. Reverting to split into a separate PR for independent review. Change-Id: Ic700ec9b537ba11e79fe746327d80da302df64db Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
已在 c18376d 中移除 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a global
--debugflag to lark-cli that enables diagnostic output to stderr viaFactory.Debugf(). This gives users a built-in way to troubleshoot command execution without modifying code or config, following the same pattern as--profileand other global flags.Changes
Debugfield toGlobalOptionsand register--debugflag incmd/global_flags.goFactory.DebugEnabledincmd/root.goDebugEnabledfield andDebugf()method (writes[DEBUG]-prefixed lines to stderr) ininternal/cmdutil/factory.go"default"as explicit profile alias ininternal/core/config.goSetInterspersed(true)for global flag parsing to support flexible flag-command-arg orderingFindAppdefault case ininternal/core/config.gocmd/global_flags_test.go) and Debugf behavior (internal/cmdutil/factory_debug_test.go)tests_e2e/cmd/2026_04_15_debug_flag_test.go)Test Plan
make validatepassedlark-cli --debug api GET /open-apis/contact/v3/users— flag parsed correctly, command succeeds with expected outputRelated Issues
N/A