feat(affordance): per-command usage guidance as per-domain markdown#1565
feat(affordance): per-command usage guidance as per-domain markdown#1565liangshuo-1 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1565 +/- ##
==========================================
+ Coverage 74.05% 74.69% +0.64%
==========================================
Files 788 810 +22
Lines 76366 81827 +5461
==========================================
+ Hits 56550 61123 +4573
- Misses 15570 16148 +578
- Partials 4246 4556 +310 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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:
📝 WalkthroughWalkthroughThe PR rewrites affordance help generation, adds markdown-based affordance overlay loading, injects overlay data into schema and command help, updates root command grouping and examples, and refreshes the ChangesAffordance overlays and CLI help wiring
Sequence Diagram(s)sequenceDiagram
participant content_embed.go
participant internal.affordance
participant cmd/service/affordance.go
participant internal/schema/assembler.go
content_embed.go->>internal.affordance: SetSource(...)
cmd/service/affordance.go->>internal.affordance: For(service, methodID)
internal.affordance-->>cmd/service/affordance.go: json.RawMessage overlay
internal.schema/assembler.go->>internal.affordance: For(ref.Service.Name, m.ID)
internal.affordance-->>internal.schema/assembler.go: overlayed affordance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
liangshuo-1
force-pushed
the
affordance-overlay
branch
from
3870612 to
b1ff52e
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9d55649a9fb3fd4b6b9d4b8ad4011bc41ae7b358🧩 Skill updatenpx skills add larksuite/cli#affordance-overlay -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/meta/affordance_test.go (1)
36-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert
AvoidWhenandPrerequisitesin the populated case.This fixture now covers the renamed
avoid_whenfield andprerequisites, but the test never verifies either value. A parser regression on those keys would still pass here.As per coding guidelines, "Every behavior change needs a test alongside the change."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/meta/affordance_test.go` around lines 36 - 59, The populated-case test in ParsedAffordance currently verifies UseWhen, Tips, Examples, and Related, but it does not assert the newly added AvoidWhen and Prerequisites fields, so a regression in those parsed keys could slip through. Update the affordance test around Method.ParsedAffordance to check that the parsed affordance includes the expected avoid_when and prerequisites values from the raw fixture, alongside the existing assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/root_test.go`:
- Around line 78-85: The current test only verifies a static footer URL and
misses the actual behavior change in root help generation. Update or add tests
around the root command help flow to cover the new grouping logic and the lazy
PrepareDomainHelp and PrepareMethodHelp paths in cmd/root.go, so regressions in
those branches are exercised instead of only checking rootUsageTemplate. Use the
existing TestRootLong_AgentSkillsLinkTargetsReadmeSection as a starting point,
but assert the command/help behavior that changes when the new branches run.
In `@cmd/root.go`:
- Around line 571-584: The command grouping logic in root.Commands() is missing
completion, so it falls into “Additional Commands” instead of the management
section. Update the classifier in cmd/root.go to treat c.Name() == "completion"
the same as the other management commands by adding it to the management mapping
used before setting c.GroupID.
In `@cmd/service/affordance.go`:
- Around line 24-65: Add tests covering the new help-generation branches so
`PrepareDomainHelp` and `PrepareMethodHelp` don’t regress silently. Exercise the
`PrepareDomainHelp` path for the shortcut/resource routing text and the optional
domain-guide line when `skillFS` contains the matching `SKILL.md`, and add
coverage for the high-risk/help workflow branches in `PrepareMethodHelp` by
asserting the expected help output changes. Use the existing `PrepareDomainHelp`
and `PrepareMethodHelp` symbols to locate the logic and verify both the gated
and non-gated cases.
In `@cmd/service/paramhelp.go`:
- Around line 33-36: The boolean help text in paramhelp.go is inaccurate because
it describes bool flags as presence-only. Update the logic in the help text
generation for CanonicalType() == "boolean" so it explains that bool flags can
be set explicitly to true or false (for example via the flag handling around
facts append), instead of implying omission is the only way to get false. Keep
the wording aligned with the existing paramhelp.go formatting and the symbol
names CanonicalType and facts.
In `@content_embed.go`:
- Around line 30-40: The warn-and-continue embed fallback in init() for
embeddedContentFS is not directly testable, and the fs.Sub failure paths for
skills and affordance are uncovered. Extract the subtree wiring into a small
helper (for example around cmd.SetEmbeddedSkillContent and affordance.SetSource)
and keep init() as a thin caller, so tests can exercise both successful fs.Sub
results and warning-only failures. Add tests for the helper to verify the
warning branches and the happy path behavior.
In `@internal/affordance/affordance_test.go`:
- Around line 31-33: The TestFor setup currently mutates the package-global
affordance source via SetSource without restoring it, which can leak state into
other tests. Update the TestFor test to save the previous source before calling
SetSource and restore it in t.Cleanup so each test leaves the global state
unchanged; use the SetSource helper and the existing test function name TestFor
to locate the setup.
In `@internal/affordance/mdparse.go`:
- Around line 141-160: Paragraph-style text in non-Examples sections is being
stored in pending but never emitted, so guidance under sections like Tips,
Prerequisites, or custom headings gets dropped. Update the section parsing in
mdparse.go around the fenced-block and pending handling so non-bullet, non-fence
lines are preserved as section content, and ensure pending is flushed into
sec.items or another emitted structure when appropriate instead of only being
consumed by meta.AffordanceCase creation. Keep the behavior consistent with the
existing parsing flow in the section parser and the linkToBacktick / sec.items
accumulation path.
In `@internal/schema/assembler_test.go`:
- Around line 513-516: The test in assembler_test.go mutates package-global
affordance source via affordance.SetSource and does not restore it, leaving
later tests dependent on execution order. Update the test to save the original
source before installing the fstest.MapFS fixture, then restore it after the
test using defer so the default source is preserved for any other tests that use
affordance or assembler_test helpers.
---
Outside diff comments:
In `@internal/meta/affordance_test.go`:
- Around line 36-59: The populated-case test in ParsedAffordance currently
verifies UseWhen, Tips, Examples, and Related, but it does not assert the newly
added AvoidWhen and Prerequisites fields, so a regression in those parsed keys
could slip through. Update the affordance test around Method.ParsedAffordance to
check that the parsed affordance includes the expected avoid_when and
prerequisites values from the raw fixture, alongside the existing assertions.
🪄 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: adcdd373-8fc8-46e1-b807-3a6104357703
📒 Files selected for processing (21)
affordance/approval.mdaffordance/mail.mdcmd/api/api.gocmd/build.gocmd/root.gocmd/root_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups_test.gocmd/service/paramhelp.gocmd/service/service.gocontent_embed.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/mdparse.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/types.goskills_embed.go
💤 Files with no reviewable changes (1)
- skills_embed.go
| tooling := map[string]bool{"api": true, "schema": true, "skills": true} | ||
| management := map[string]bool{"auth": true, "config": true, "profile": true, "doctor": true, "update": true} | ||
| for _, c := range root.Commands() { | ||
| if c.GroupID != "" { | ||
| continue | ||
| } | ||
| switch { | ||
| case tooling[c.Name()]: | ||
| c.GroupID = groupTooling | ||
| case management[c.Name()]: | ||
| c.GroupID = groupManagement | ||
| case isLarkDomain(c): | ||
| c.GroupID = groupDomains | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Group completion with the other management commands.
buildInternal still registers completion as a root child, but this classifier never assigns it a group, so it will drop into Cobra’s “Additional Commands” block instead of the new curated sections.
Suggested fix
- management := map[string]bool{"auth": true, "config": true, "profile": true, "doctor": true, "update": true}
+ management := map[string]bool{"auth": true, "config": true, "profile": true, "doctor": true, "update": true, "completion": true}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tooling := map[string]bool{"api": true, "schema": true, "skills": true} | |
| management := map[string]bool{"auth": true, "config": true, "profile": true, "doctor": true, "update": true} | |
| for _, c := range root.Commands() { | |
| if c.GroupID != "" { | |
| continue | |
| } | |
| switch { | |
| case tooling[c.Name()]: | |
| c.GroupID = groupTooling | |
| case management[c.Name()]: | |
| c.GroupID = groupManagement | |
| case isLarkDomain(c): | |
| c.GroupID = groupDomains | |
| } | |
| tooling := map[string]bool{"api": true, "schema": true, "skills": true} | |
| management := map[string]bool{"auth": true, "config": true, "profile": true, "doctor": true, "update": true, "completion": true} | |
| for _, c := range root.Commands() { | |
| if c.GroupID != "" { | |
| continue | |
| } | |
| switch { | |
| case tooling[c.Name()]: | |
| c.GroupID = groupTooling | |
| case management[c.Name()]: | |
| c.GroupID = groupManagement | |
| case isLarkDomain(c): | |
| c.GroupID = groupDomains | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 575-575: cmd/root.go#L575
Added line #L575 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/root.go` around lines 571 - 584, The command grouping logic in
root.Commands() is missing completion, so it falls into “Additional Commands”
instead of the management section. Update the classifier in cmd/root.go to treat
c.Name() == "completion" the same as the other management commands by adding it
to the management mapping used before setting c.GroupID.
| func PrepareDomainHelp(cmd *cobra.Command, skillFS fs.FS) bool { | ||
| if cmd.Annotations[schemaPathAnnotation] != "" { | ||
| return false // a method command | ||
| } | ||
| // Direct child of root only — so Domain() reads this command's own tag, and | ||
| // nested resource groups are excluded. | ||
| if cmd.Parent() == nil || cmd.Parent().Parent() != nil { | ||
| return false | ||
| } | ||
| // A domain is service-sourced or shortcut-tagged; CLI tooling has neither. | ||
| if src, _ := cmdmeta.SourceOf(cmd); src != cmdmeta.SourceService && cmdmeta.Domain(cmd) == "" { | ||
| return false | ||
| } | ||
| if !cmd.HasAvailableSubCommands() { | ||
| return false | ||
| } | ||
|
|
||
| hasShortcuts, hasResources := false, false | ||
| for _, c := range cmd.Commands() { | ||
| if c.Hidden || c.Name() == "help" || c.Name() == "completion" { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(c.Name(), "+") { | ||
| hasShortcuts = true | ||
| } else { | ||
| hasResources = true | ||
| } | ||
| } | ||
|
|
||
| var b strings.Builder | ||
| b.WriteString(description) | ||
| if affordance != "" { | ||
| b.WriteString("\n\n") | ||
| b.WriteString(affordance) | ||
| b.WriteString(cmd.Short) | ||
| if hasShortcuts && hasResources { // routing only matters when both styles exist | ||
| b.WriteString("\n\nPrefer a +-prefixed shortcut when one matches your task; otherwise use the raw API resource below.") | ||
| } | ||
| fmt.Fprintf(&b, "\n\nView parameter definitions before calling:\n lark-cli schema %s", schemaPath) | ||
| b.WriteString("\n\nRisk levels (read | write | high-risk-write) appear in each command's --help; high-risk-write requires --yes, only after the user confirms.") | ||
| if skill := "lark-" + cmd.Name(); skillFS != nil { | ||
| if _, err := fs.Stat(skillFS, skill+"/SKILL.md"); err == nil { | ||
| fmt.Fprintf(&b, "\n\nDomain guide (concepts, command choice, conventions): lark-cli skills read %s", skill) | ||
| } | ||
| } | ||
| cmd.Long = b.String() | ||
| return true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add tests for the new lazy-help branches.
PrepareDomainHelp is new behavior, and the added high-risk / workflow-skill branches in PrepareMethodHelp are still uncovered. That leaves the CLI help contract easy to regress without any failing test. As per coding guidelines, “Every behavior change needs a test alongside the change.”
Also applies to: 119-145
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-26: cmd/service/affordance.go#L24-L26
Added lines #L24 - L26 were not covered by tests
[warning] 30-31: cmd/service/affordance.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 34-35: cmd/service/affordance.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 37-38: cmd/service/affordance.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 41-44: cmd/service/affordance.go#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-49: cmd/service/affordance.go#L46-L49
Added lines #L46 - L49 were not covered by tests
[warning] 54-56: cmd/service/affordance.go#L54-L56
Added lines #L54 - L56 were not covered by tests
[warning] 58-61: cmd/service/affordance.go#L58-L61
Added lines #L58 - L61 were not covered by tests
[warning] 64-65: cmd/service/affordance.go#L64-L65
Added lines #L64 - L65 were not covered by tests
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/service/affordance.go` around lines 24 - 65, Add tests covering the new
help-generation branches so `PrepareDomainHelp` and `PrepareMethodHelp` don’t
regress silently. Exercise the `PrepareDomainHelp` path for the
shortcut/resource routing text and the optional domain-guide line when `skillFS`
contains the matching `SKILL.md`, and add coverage for the high-risk/help
workflow branches in `PrepareMethodHelp` by asserting the expected help output
changes. Use the existing `PrepareDomainHelp` and `PrepareMethodHelp` symbols to
locate the logic and verify both the gated and non-gated cases.
Sources: Coding guidelines, Linters/SAST tools
| if f.CanonicalType() == "boolean" { | ||
| // cobra shows no type word for bools and swallows a separate value as a | ||
| // positional, so spell out the presence-only contract. | ||
| facts = append(facts, "bool flag (presence = true; omit for false; takes no value)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Does github.com/spf13/pflag support boolean flags being passed explicitly as --flag=false / --flag=true? Please cite the official documentation or source behavior.
💡 Result:
Yes, the spf13/pflag library supports passing boolean flags explicitly using the --flag=true or --flag=false syntax [1][2][3]. According to the official documentation and command line syntax specifications for pflag, boolean flags in their long form accept the values 1, 0, t, f, true, false, TRUE, FALSE, True, and False when provided with the equals sign syntax (e.g., --flag=false) [2][3][4]. However, pflag does not support passing boolean values as separate arguments (e.g., --flag false), which is considered invalid syntax [2][3]. The library is designed such that providing the flag alone (e.g., --flag) sets it to true [2][3][4]. For shorthand flags, passing an argument (e.g., -b true) is also invalid; shorthand flags are intended to be toggles [2][3].
Citations:
- 1: Please allow booleans to be set false with
no-prefix spf13/cobra#958 - 2: https://github.com/spf13/pflag
- 3: https://pkg.go.dev/github.com/spf13/pflag
- 4: https://chromium.googlesource.com/external/github.com/spf13/pflag/+/252d3ee662414967926c300a620758cb1dc452e2/flag.go
🏁 Script executed:
sed -n '1,120p' cmd/service/paramhelp.goRepository: larksuite/cli
Length of output: 1909
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('cmd/service/paramhelp.go')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 120:
print(f"{i:4d}: {line}")
PYRepository: larksuite/cli
Length of output: 1909
Don't describe bool flags as presence-only. bool flags accept explicit values like --flag=false, so this help text should say how to set both true and false instead of implying omission is the only way to get false.
Suggested wording
- facts = append(facts, "bool flag (presence = true; omit for false; takes no value)")
+ facts = append(facts, "bool flag (use --flag to set true; pass --flag=false to set false)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if f.CanonicalType() == "boolean" { | |
| // cobra shows no type word for bools and swallows a separate value as a | |
| // positional, so spell out the presence-only contract. | |
| facts = append(facts, "bool flag (presence = true; omit for false; takes no value)") | |
| if f.CanonicalType() == "boolean" { | |
| // cobra shows no type word for bools and swallows a separate value as a | |
| // positional, so spell out the presence-only contract. | |
| facts = append(facts, "bool flag (use --flag to set true; pass --flag=false to set false)") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/service/paramhelp.go` around lines 33 - 36, The boolean help text in
paramhelp.go is inaccurate because it describes bool flags as presence-only.
Update the logic in the help text generation for CanonicalType() == "boolean" so
it explains that bool flags can be set explicitly to true or false (for example
via the flag handling around facts append), instead of implying omission is the
only way to get false. Keep the wording aligned with the existing paramhelp.go
formatting and the symbol names CanonicalType and facts.
| func init() { | ||
| if sub, err := fs.Sub(embeddedContentFS, "skills"); err != nil { | ||
| fmt.Fprintln(os.Stderr, "warning: skills embed assembly failed, skills commands disabled:", err) | ||
| } else { | ||
| cmd.SetEmbeddedSkillContent(sub) | ||
| } | ||
| if sub, err := fs.Sub(embeddedContentFS, "affordance"); err != nil { | ||
| fmt.Fprintln(os.Stderr, "warning: affordance embed assembly failed, command guidance disabled:", err) | ||
| } else { | ||
| affordance.SetSource(sub) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the warning-only embed fallback testable.
The new “warn and continue” behavior for missing skills / affordance subtrees is load-bearing, but the failure branches are currently untested. Pulling this wiring into a small helper and calling it from init() would let us cover both fs.Sub failures and the happy path. As per coding guidelines: "Every behavior change needs a test alongside the change."
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: content_embed.go#L32
Added line #L32 was not covered by tests
[warning] 37-37: content_embed.go#L37
Added line #L37 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@content_embed.go` around lines 30 - 40, The warn-and-continue embed fallback
in init() for embeddedContentFS is not directly testable, and the fs.Sub failure
paths for skills and affordance are uncovered. Extract the subtree wiring into a
small helper (for example around cmd.SetEmbeddedSkillContent and
affordance.SetSource) and keep init() as a thin caller, so tests can exercise
both successful fs.Sub results and warning-only failures. Add tests for the
helper to verify the warning branches and the happy path behavior.
Sources: Coding guidelines, Linters/SAST tools
liangshuo-1
force-pushed
the
affordance-overlay
branch
from
b1ff52e to
8d3a2b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@affordance/mail.md`:
- Around line 42-52: The EML guidance in the mailbox draft docs uses the wrong
RFC number and should reference the email message format standard instead.
Update the text in the user_mailbox.drafts create section, and any related
draft/send/reply guidance it points to, so every occurrence of RFC 5822 becomes
RFC 5322 in the mail documentation. Keep the wording otherwise unchanged and use
the existing user_mailbox.drafts create labels to locate all matching
references.
🪄 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: b95e637b-3d8a-4830-8cfe-198f1061c255
📒 Files selected for processing (21)
affordance/approval.mdaffordance/mail.mdcmd/api/api.gocmd/build.gocmd/root.gocmd/root_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups_test.gocmd/service/paramhelp.gocmd/service/service.gocontent_embed.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/mdparse.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/types.goskills_embed.go
💤 Files with no reviewable changes (1)
- skills_embed.go
✅ Files skipped from review due to trivial changes (1)
- cmd/api/api.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/build.go
- content_embed.go
- cmd/service/flaggroups_test.go
- cmd/root_test.go
- internal/schema/assembler_test.go
- cmd/service/paramhelp.go
- internal/schema/types.go
- cmd/root.go
- internal/meta/affordance_test.go
- internal/affordance/affordance.go
- internal/affordance/mdparse.go
- internal/meta/affordance.go
- cmd/service/service.go
- cmd/service/affordance.go
- internal/schema/assembler.go
- cmd/service/affordance_test.go
| ## user_mailbox.drafts create | ||
| 创建一封邮件草稿（不立即发送），且你已有现成的 RFC 5822(EML) 原文 | ||
|
|
||
| ### Avoid when | ||
| - 想直接撰写新邮件而不手工拼 base64 MIME（起草用 [[+draft-create]]，撰写并发送用 [[+send]]，回复用 [[+reply]] —— 这些高层命令默认存草稿，加 --confirm-send 才真正发送） | ||
| - 要修改已有草稿（用 [[user_mailbox.drafts update]]） | ||
| - 要把草稿发出去（先 create/update 再用 [[user_mailbox.drafts send]]） | ||
|
|
||
| ### Tips | ||
| - raw 为 base64url 编码的完整 RFC 5822(EML) 邮件内容，含所有邮件头(Subject/From/To/Cc/Bcc)与正文 | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the RFC number in the EML guidance.
RFC 5822 is not the standard email/message-format reference here; these sections appear to mean RFC 5322. Please update all occurrences so the docs point readers to the correct spec.
♻️ Proposed fix
-且你已有现成的 RFC 5822(EML) 原文
+且你已有现成的 RFC 5322(EML) 原文
@@
-- raw 为 base64url 编码的完整 RFC 5822(EML) 邮件内容，含所有邮件头(Subject/From/To/Cc/Bcc)与正文
+- raw 为 base64url 编码的完整 RFC 5322(EML) 邮件内容，含所有邮件头(Subject/From/To/Cc/Bcc)与正文Also applies to: 133-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@affordance/mail.md` around lines 42 - 52, The EML guidance in the mailbox
draft docs uses the wrong RFC number and should reference the email message
format standard instead. Update the text in the user_mailbox.drafts create
section, and any related draft/send/reply guidance it points to, so every
occurrence of RFC 5822 becomes RFC 5322 in the mail documentation. Keep the
wording otherwise unchanged and use the existing user_mailbox.drafts create
labels to locate all matching references.
8d3a2b2 to
4c89e41
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
affordance/mail.md (1)
43-43: 📐 Maintainability & Code Quality | 🟡 MinorFix the RFC number in the EML guidance.
RFC 5822is not the standard email/message-format reference; these sections meanRFC 5322. Please update all occurrences so the docs point readers to the correct spec.This was previously flagged but still appears unfixed in the current revision.
♻️ Proposed fix
-创建一封邮件草稿（不立即发送），且你已有现成的 RFC 5822(EML) 原文 +创建一封邮件草稿（不立即发送），且你已有现成的 RFC 5322(EML) 原文 @@ -- raw 为 base64url 编码的完整 RFC 5822(EML) 邮件内容，含所有邮件头(Subject/From/To/Cc/Bcc)与正文 +- raw 为 base64url 编码的完整 RFC 5322(EML) 邮件内容，含所有邮件头(Subject/From/To/Cc/Bcc)与正文 @@ -- raw 为 base64url 编码的完整 RFC 5822(EML) 邮件内容，全量替换 +- raw 为 base64url 编码的完整 RFC 5322(EML) 邮件内容，全量替换Also applies to: 51-51, 144-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@affordance/mail.md` at line 43, The EML guidance uses the wrong RFC number, so update every mention of RFC 5822 to RFC 5322 in the mail guidance content. Make the change consistently across the affected text in the mail affordance docs, especially the sections that describe using existing EML source and creating a draft without sending, so readers are pointed to the correct message-format spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@affordance/mail.md`:
- Line 43: The EML guidance uses the wrong RFC number, so update every mention
of RFC 5822 to RFC 5322 in the mail guidance content. Make the change
consistently across the affected text in the mail affordance docs, especially
the sections that describe using existing EML source and creating a draft
without sending, so readers are pointed to the correct message-format spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff3ecb3e-b4a2-4f3a-b43d-c6fd7dfa005e
📒 Files selected for processing (21)
affordance/approval.mdaffordance/mail.mdcmd/api/api.gocmd/build.gocmd/root.gocmd/root_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups_test.gocmd/service/paramhelp.gocmd/service/service.gocontent_embed.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/mdparse.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/types.goskills_embed.go
💤 Files with no reviewable changes (1)
- skills_embed.go
✅ Files skipped from review due to trivial changes (1)
- cmd/api/api.go
🚧 Files skipped from review as they are similar to previous changes (17)
- cmd/service/flaggroups_test.go
- cmd/build.go
- internal/affordance/affordance_test.go
- cmd/root_test.go
- internal/meta/affordance_test.go
- internal/schema/types.go
- content_embed.go
- internal/schema/assembler_test.go
- cmd/root.go
- internal/meta/affordance.go
- cmd/service/affordance_test.go
- internal/schema/assembler.go
- internal/affordance/affordance.go
- internal/affordance/mdparse.go
- cmd/service/service.go
- cmd/service/paramhelp.go
- cmd/service/affordance.go
liangshuo-1
force-pushed
the
affordance-overlay
branch
2 times, most recently
from
b952301 to
c906908
Compare
Adds the affordance mechanism: per-command usage guidance authored as one markdown file per domain under affordance/, surfaced in `--help` and the schema output and read directly at runtime (lazy, cached). See affordance/README.md for the format; contact is included as a worked example. Remaining domains land in follow-up changes. Also trims a couple of redundant lines from the contact skill.
liangshuo-1
force-pushed
the
affordance-overlay
branch
from
c906908 to
9d55649
Compare
1 participant
Adds per-command usage guidance (affordance) authored as one markdown file per domain under
affordance/, surfaced in--helpand theschemaoutput and read directly at runtime. Covers all domains.Summary by CodeRabbit
New Features
Bug Fixes