feat: add framework-level output projection with --full#1537
Open
luozhixiong01 wants to merge 1 commit into
Open
feat: add framework-level output projection with --full#1537luozhixiong01 wants to merge 1 commit into
luozhixiong01 wants to merge 1 commit into
Conversation
Adds a domain-agnostic output-projection engine to the shortcut runtime; a Projectable shortcut declares an OutputSchema and the runtime trims the default view to the curated whitelist, with a boolean --full to restore the full upstream payload. The 5 im read shortcuts are the first adopters; other domains are unchanged (projection is opt-in via OutputSchema). Message senders are tightened to {id, sender_type, name}; adds a jq-miss stderr hint.
github-actions
Bot
added
domain/im
📝 WalkthroughWalkthroughAdds a schema-driven output projection framework to ChangesOutput Projection Framework
Sequence DiagramsequenceDiagram
participant CLI as CLI User
participant runner as runner emit
participant ProjectBySchema as ProjectBySchema
participant JqFilterCount as JqFilterCount
participant stderr as stderr
CLI->>runner: run shortcut with --jq expression
runner->>ProjectBySchema: project response data
ProjectBySchema-->>runner: trimmed data
runner->>JqFilterCount: apply jq expression to trimmed data
JqFilterCount-->>runner: result count and error
runner->>stderr: emit full-only field hint when count is zero
runner-->>CLI: projected stdout output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 1
🧹 Nitpick comments (2)
shortcuts/common/projection_test.go (1)
178-200: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd an array-item nested case to
TestAnyProjected.Current coverage validates nested object traversal but not
Items.Properties. Please add one assertion for a schema where only array-element fields are marked projected, so guard behavior is locked for both object and array trees.Suggested test addition
func TestAnyProjected(t *testing.T) { none := &schema.OrderedProps{} none.Set("a", schema.Property{}) if anyProjected(none) { t.Fatalf("no marks should report false") } @@ if anyProjected(nil) { t.Fatalf("nil should report false") } + + arrElem := &schema.OrderedProps{} + arrElem.Set("id", schema.Property{Projected: true}) + arr := &schema.OrderedProps{} + arr.Set("items", schema.Property{ + Type: "array", + Items: &schema.Property{Type: "object", Properties: arrElem}, + }) + if !anyProjected(arr) { + t.Fatalf("array item nested mark should report true") + } }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 `@shortcuts/common/projection_test.go` around lines 178 - 200, The TestAnyProjected function needs an additional test case to verify that the anyProjected function correctly handles nested array items. Add a new assertion within the TestAnyProjected function that creates a schema.OrderedProps with a property of type "array", sets Items with Properties containing a schema.Property marked with Projected: true, then verifies that anyProjected returns true for this nested array case. This ensures guard behavior is tested for both object and array tree traversal, not just object nesting.Source: Coding guidelines
shortcuts/im/convert_lib/helpers.go (1)
212-213: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSkip already-attempted IDs by key presence, not non-empty value.
Line 212 re-queues IDs when
nameMap[id] == "", so previously attempted-but-unresolved users can be queried repeatedly acrossResolveSenderNamescalls.Proposed patch
func unresolvedUserSenderIDs(messages []map[string]interface{}, nameMap map[string]string) []string { var ids []string seen := map[string]bool{} for _, msg := range messages { s, _ := msg["sender"].(map[string]interface{}) id, _ := s["id"].(string) - if id == "" || !strings.HasPrefix(id, "ou_") || seen[id] || nameMap[id] != "" { + if id == "" || !strings.HasPrefix(id, "ou_") || seen[id] { + continue + } + if _, attempted := nameMap[id]; attempted { continue } seen[id] = true ids = append(ids, id) } return ids }🤖 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 `@shortcuts/im/convert_lib/helpers.go` around lines 212 - 213, The condition on line 212 currently checks nameMap[id] != "" to skip IDs that have been processed, but this causes previously attempted-but-unresolved users to be queried repeatedly. Instead of checking for a non-empty value in nameMap, use Go's comma-ok idiom to check if the key exists in the map. Replace the nameMap[id] != "" check with a proper key existence check using the pattern _, exists := nameMap[id], then only skip the ID if the key is present in the map, regardless of whether the value is empty or not.
🤖 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 `@shortcuts/common/projection.go`:
- Around line 132-149: The anyProjected function fails to check for projected
marks nested under array item properties, only examining p.Properties during
recursion. After the recursive call to anyProjected for p.Properties in the
function body, add an additional check that also recursively calls anyProjected
on p.Items.Properties if p.Items is not nil. This ensures that projected fields
within array elements are properly detected and prevents schemas that project
only array element fields from being incorrectly treated as pass-through.
---
Nitpick comments:
In `@shortcuts/common/projection_test.go`:
- Around line 178-200: The TestAnyProjected function needs an additional test
case to verify that the anyProjected function correctly handles nested array
items. Add a new assertion within the TestAnyProjected function that creates a
schema.OrderedProps with a property of type "array", sets Items with Properties
containing a schema.Property marked with Projected: true, then verifies that
anyProjected returns true for this nested array case. This ensures guard
behavior is tested for both object and array tree traversal, not just object
nesting.
In `@shortcuts/im/convert_lib/helpers.go`:
- Around line 212-213: The condition on line 212 currently checks nameMap[id] !=
"" to skip IDs that have been processed, but this causes previously
attempted-but-unresolved users to be queried repeatedly. Instead of checking for
a non-empty value in nameMap, use Go's comma-ok idiom to check if the key exists
in the map. Replace the nameMap[id] != "" check with a proper key existence
check using the pattern _, exists := nameMap[id], then only skip the ID if the
key is present in the map, regardless of whether the value is empty or not.
🪄 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: 79cf9a17-1e58-4e84-b997-8391f898bb1a
📒 Files selected for processing (27)
internal/output/jq.gointernal/schema/types.goshortcuts/common/projection.goshortcuts/common/projection_test.goshortcuts/common/runner.goshortcuts/common/types.goshortcuts/im/convert_lib/content_convert.goshortcuts/im/convert_lib/helpers.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_search.goshortcuts/im/im_feed_group_list.goshortcuts/im/im_feed_shortcut_list.goshortcuts/im/im_flag_list.goshortcuts/im/im_messages_mget.goshortcuts/im/im_messages_search.goshortcuts/im/im_threads_messages_list.goskills/lark-im/references/lark-im-chat-list.mdskills/lark-im/references/lark-im-chat-messages-list.mdskills/lark-im/references/lark-im-chat-search.mdskills/lark-im/references/lark-im-feed-group-list.mdskills/lark-im/references/lark-im-feed-shortcut-list.mdskills/lark-im/references/lark-im-flag-list.mdskills/lark-im/references/lark-im-messages-mget.mdskills/lark-im/references/lark-im-messages-search.mdskills/lark-im/references/lark-im-threads-messages-list.mdskills/lark-shared/SKILL.md
Comment on lines
+132
to
+149
| // anyProjected reports whether any field in the tree carries Projected==true. | ||
| // Used as a guard: a Projectable command whose OutputSchema marks nothing is | ||
| // treated as pass-through rather than trimming everything away. | ||
| func anyProjected(props *schema.OrderedProps) bool { | ||
| if props == nil { | ||
| return false | ||
| } | ||
| for _, key := range props.Order { | ||
| p := props.Map[key] | ||
| if p.Projected { | ||
| return true | ||
| } | ||
| if anyProjected(p.Properties) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
anyProjected misses projected marks nested under array items.
At Line 144, recursion only checks p.Properties; it ignores p.Items.Properties. That makes anyProjected return false for schemas that project only array element fields, so projection can be incorrectly treated as pass-through.
Proposed fix
func anyProjected(props *schema.OrderedProps) bool {
if props == nil {
return false
}
for _, key := range props.Order {
p := props.Map[key]
if p.Projected {
return true
}
- if anyProjected(p.Properties) {
+ if anyProjected(childProps(p)) {
return true
}
}
return 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 `@shortcuts/common/projection.go` around lines 132 - 149, The anyProjected
function fails to check for projected marks nested under array item properties,
only examining p.Properties during recursion. After the recursive call to
anyProjected for p.Properties in the function body, add an additional check that
also recursively calls anyProjected on p.Items.Properties if p.Items is not nil.
This ensures that projected fields within array elements are properly detected
and prevents schemas that project only array element fields from being
incorrectly treated as pass-through.
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
1 participant
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
Adds a framework-level (domain-agnostic) output-projection capability to the shortcut runtime: a Projectable shortcut declares an
OutputSchema, the runtime trims the default view to the curated whitelist, and a boolean--fullescape hatch restores the complete upstream payload. The default view is a positive whitelist — fields not declared (including future upstream additions) are full-only. im read shortcuts are the first adopters (verbose fields like chatavatarURLs,tenant_key, and messagesenderinternals waste agent context); other domains' read shortcuts are unchanged, since projection is opt-in per shortcut. Measured:+chat-listdefault is ~46% smaller than--full.Changes
shortcuts/common/projection.go(ProjectBySchemanormalizes to canonical JSON, then keeps onlyProjectedfields;KeepFields/ArrayOf/ObjectOfbuilders) + unit testsprojection_test.goshortcuts/common/runner.go(emit/outFormat apply projection, register--full, jq-miss stderr hint),types.go(Projectable/OutputSchema/NoFullViewHint),internal/schema/types.go(Projectedtag),internal/output/jq.go(jq filter counts)OutputSchemaon 5 im read shortcuts:+chat-list,+chat-search,+flag-list,+feed-shortcut-list,+feed-group-listsenderto{id, sender_type, name}and decouple name resolution inshortcuts/im/convert_lib/{content_convert,helpers}.go; add a no-full redirect hint on 4 message commandsskills/lark-im/references/*.md(curated-view /--full --jqusage) and add a curated-view/--fullconvention toskills/lark-shared/SKILL.mdOutputSchema; only the 5 im read shortcuts above adopt it in this PR — all other domains' read output is unchangedTest Plan
make unit-testpassedlark-cli im +chat-list --as user --page-size 20 --format jsonreturns 6554 B (curated) vs 12146 B with--full(46% smaller default);--full --jq '.data.chats[].avatar'retrieves the hidden fieldRelated Issues
N/A
Summary by CodeRabbit
New Features
--fullflag support to reveal complete output for chat-list, chat-search, feed-group-list, feed-shortcut-list, and flag-list commands.--fullto access all available data.--fullwhen jq filters reference fields not in the default view.Documentation