feat: add TestGenerateShortcutsJSON and skip redundant meta fetch#179
feat: add TestGenerateShortcutsJSON and skip redundant meta fetch#179MaxHuang22 merged 2 commits intomainfrom
Conversation
Add a test that exports all shortcuts as JSON when SHORTCUTS_OUTPUT env var is set, enabling the registry repo to extract shortcut metadata without depending on a dump-shortcuts CLI command. Change-Id: I9e450fdcb579d6a21ac8ec36e4197728bb7408bb Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA script ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces two quality-of-life improvements: a Key changes and observations:
Confidence Score: 4/5Safe to merge for build functionality, but the generated shortcuts JSON may contain null scopes and non-deterministic ordering, making it unreliable as a machine-readable artifact for the downstream registry. The fetch_meta.py skip logic is solid and the --force escape hatch is a good addition. However, two data-correctness issues in the primary deliverable (the shortcuts JSON generator) flagged in the prior review remain open: bot-only shortcuts emit "scopes": null instead of [] and map iteration produces different key order on every run. If the downstream registry diffs or commits this file, both issues will cause recurring noise or incorrect data. These warrant resolution before the test is put into active use. shortcuts/register_test.go — nil-scopes normalisation and deterministic key ordering should be addressed before the generated file is consumed by the registry repo. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fetch_meta.py invoked] --> B{--force flag?}
B -- Yes --> F[fetch_remote brand]
B -- No --> C{OUT_PATH exists?}
C -- No --> F
C -- Yes --> D{is a file?}
D -- No --> F
D -- Yes --> E{Valid JSON with services key?}
E -- No / empty --> F
E -- Yes --> G[Skip: print msg to stderr, return 0]
F --> H[Write meta_data.json to disk]
Reviews (2): Last reviewed commit: "fix: skip fetch_meta.py when meta_data.j..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0d0c64dbf8cbe03d1dd2cad187427f5ab1c2fd42🧩 Skill updatenpx skills add larksuite/cli#feat/gen-skill -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/fetch_meta.py (1)
67-74:⚠️ Potential issue | 🟠 MajorValidate existing metadata before skipping, and add a force refresh path.
Line 72 currently checks only path existence. If
internal/registry/meta_data.jsonis corrupt/empty (or not a regular file), fetch is skipped and build keeps invalid metadata. Also, this makes explicit refreshes awkward when--brandchanges.Suggested patch
def main(): parser = argparse.ArgumentParser(description="Fetch meta_data.json for build-time embedding") parser.add_argument("--brand", default="feishu", choices=["feishu", "lark"], help="API brand (default: feishu)") + parser.add_argument("--force", action="store_true", + help="force refresh from remote even if local file exists") args = parser.parse_args() - if os.path.exists(OUT_PATH): - print(f"fetch-meta: {OUT_PATH} already exists, skipping (delete to re-fetch)", file=sys.stderr) - return + if os.path.exists(OUT_PATH) and not args.force: + if os.path.isfile(OUT_PATH): + try: + with open(OUT_PATH, "r", encoding="utf-8") as fp: + local = json.load(fp) + if local.get("services"): + print(f"fetch-meta: {OUT_PATH} already exists, skipping (use --force to re-fetch)", file=sys.stderr) + return + print(f"fetch-meta: {OUT_PATH} has no services, re-fetching", file=sys.stderr) + except (OSError, json.JSONDecodeError): + print(f"fetch-meta: {OUT_PATH} is invalid JSON, re-fetching", file=sys.stderr) + else: + print(f"fetch-meta: {OUT_PATH} is not a file, re-fetching", file=sys.stderr) data = fetch_remote(args.brand)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fetch_meta.py` around lines 67 - 74, The existing skip logic only checks OUT_PATH existence; change it to validate it's a regular file (os.path.isfile), non-empty (os.path.getsize>0), and that its contents parse as valid JSON and — if present — contain a matching "brand" field equal to args.brand; add a --force (or -f) boolean argparse flag to override validation and force re-fetch. In practice update the parser to include parser.add_argument("--force", action="store_true", help="force refresh"), then replace the current if os.path.exists(OUT_PATH) check with a block that returns only when not args.force and the file is a regular non-empty file and json.load(open(OUT_PATH)) succeeds (and the parsed_data.get("brand")==args.brand); otherwise proceed to re-fetch and overwrite OUT_PATH.
🧹 Nitpick comments (1)
shortcuts/register_test.go (1)
104-116: Include bot identity scopes in exported shortcuts registry.Line 115 exports only user scopes via
s.ScopesForIdentity("user"). At least 4 shortcuts declare distinct bot scopes (e.g.,im_threads_messages_list,contact_get_user), and 4 shortcuts are bot-only (e.g.,im_chat_create,event/subscribe). The exported JSON loses this metadata.Update the entry struct to include scopes for all declared identities in
AuthTypes:Suggested schema change
type entry struct { Verb string `json:"verb"` Description string `json:"description"` - Scopes []string `json:"scopes"` + Scopes map[string][]string `json:"scopes"` } @@ grouped[s.Service] = append(grouped[s.Service], entry{ Verb: verb, Description: s.Description, - Scopes: s.ScopesForIdentity("user"), + Scopes: func() map[string][]string { + scopes := make(map[string][]string) + for _, id := range s.AuthTypes { + scopes[id] = s.ScopesForIdentity(id) + } + return scopes + }(), }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/register_test.go` around lines 104 - 116, The exported entry currently only includes user scopes; update the entry struct (the type entry) to include a map of scopes per identity (e.g., ScopesByIdentity map[string][]string) instead of (or in addition to) the single Scopes field, then in the loop over shortcuts (for _, s := range shortcuts) populate that map by iterating the identities in AuthTypes and calling s.ScopesForIdentity(identity) for each identity so bot scopes (and any other identities) are preserved in grouped[s.Service] entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/fetch_meta.py`:
- Around line 67-74: The existing skip logic only checks OUT_PATH existence;
change it to validate it's a regular file (os.path.isfile), non-empty
(os.path.getsize>0), and that its contents parse as valid JSON and — if present
— contain a matching "brand" field equal to args.brand; add a --force (or -f)
boolean argparse flag to override validation and force re-fetch. In practice
update the parser to include parser.add_argument("--force", action="store_true",
help="force refresh"), then replace the current if os.path.exists(OUT_PATH)
check with a block that returns only when not args.force and the file is a
regular non-empty file and json.load(open(OUT_PATH)) succeeds (and the
parsed_data.get("brand")==args.brand); otherwise proceed to re-fetch and
overwrite OUT_PATH.
---
Nitpick comments:
In `@shortcuts/register_test.go`:
- Around line 104-116: The exported entry currently only includes user scopes;
update the entry struct (the type entry) to include a map of scopes per identity
(e.g., ScopesByIdentity map[string][]string) instead of (or in addition to) the
single Scopes field, then in the loop over shortcuts (for _, s := range
shortcuts) populate that map by iterating the identities in AuthTypes and
calling s.ScopesForIdentity(identity) for each identity so bot scopes (and any
other identities) are preserved in grouped[s.Service] entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fef8427f-569d-4e0a-a912-d4a54019fa78
📒 Files selected for processing (2)
scripts/fetch_meta.pyshortcuts/register_test.go
Avoid overwriting a locally generated meta_data.json with the remote version during build. Change-Id: Ic30d70b9fa9d9c8e060879e4000c53c58f3aa525 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MaxHuang22
force-pushed
the
feat/gen-skill
branch
from
88e6a9c to
0d0c64d
Compare
* feat: add TestGenerateShortcutsJSON for registry shortcut export Add a test that exports all shortcuts as JSON when SHORTCUTS_OUTPUT env var is set, enabling the registry repo to extract shortcut metadata without depending on a dump-shortcuts CLI command.
2 participants
Summary
TestGenerateShortcutsJSONinshortcuts/register_test.go: exports all shortcuts as JSON whenSHORTCUTS_OUTPUTenv var is set, enabling the registry repo to extract shortcut metadata without adump-shortcutsCLI command. Skips automatically when env is not set (zero CI impact).scripts/fetch_meta.pyto skip downloading whenmeta_data.jsonalready exists locally, preventing overwrite of registry-generated versions during build.Test plan
go test ./shortcuts/...— all existing tests pass,TestGenerateShortcutsJSONshows SKIPSHORTCUTS_OUTPUT=/tmp/test.json go test -run TestGenerateShortcutsJSON ./shortcuts/...— generates valid JSON./build.shwith existingmeta_data.json— skips fetch, prints skip message./build.shwithoutmeta_data.json— fetches from remote as before🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests