perf(startup): static typed command registry + keychain-free startup#1387
Open
sang-neo03 wants to merge 6 commits into
Open
perf(startup): static typed command registry + keychain-free startup#1387sang-neo03 wants to merge 6 commits into
sang-neo03 wants to merge 6 commits into
Conversation
Render the embedded command spec as static Go data (metaschema types + metastatic generator and stub) to eliminate the startup JSON parse and allocation. Behind the larkmeta build tag; the runtime still uses the JSON path (not yet wired). A validation test confirms the static data matches the JSON parse (13 services / 70 resources / 215 methods / 4233 fields) and that reads allocate nothing. Generated meta_data_gen.go is gitignored, like meta_data.json.
Migrate the startup command-tree build path to read compile-time static
Go structs (metastatic.Registry, behind -tags larkmeta) instead of parsing
the 1.9MB meta_data.json into map[string]interface{} (~170K heap objects)
on every invocation.
- typed.go: typed baseline + remote-overlay layer. Reads static data under
-tags larkmeta (zero parse / zero alloc); falls back to parsing the
embedded meta_data.json once for builds without the tag (dev/test). A lazy
typed->map shim (ServiceToMap/MethodToMap) keeps execution-path consumers
working unchanged.
- service.go: registration chain and NewCmdServiceMethod read typed structs
directly; map wrappers convert via MapToService/MapToMethod for existing
tests; method RunE materializes opts.Spec/opts.Method lazily, never at
startup.
- loader.go/remote.go: baseline and cached remote overlay decode into the
typed shape; runtime refresh preserved.
- build pipeline: Makefile, goreleaser and pkg-pr-new run the generator and
build with -tags larkmeta so shipped binaries carry the static data.
Reading the full static registry is 0 B/op, 0 allocs/op; the built command
tree is byte-identical to the JSON-parsed tree (verified via a canonical
tree dump in both modes).
The startup baseline now comes solely from the generated static Go registry (metastatic.Registry), wired into the stub-declared Registry via a package-level var plus an init() struct-header copy. No build tag, no committed generated file, and zero startup allocation is preserved. - gen.go emits a tag-free `var registryData` + `func init()` instead of a //go:build larkmeta top-level `var Registry`; stub.go declares Registry unconditionally so the package always compiles - fetch_meta.py regenerates the static registry after fetching, so every build and CI step that fetches also produces it (no separate gen step, no CI change) - remove the //go:embed meta_data.json baseline and the JSON parse fallback; meta_data.json is now only the build-time input to the generator - EmbeddedSpec/EmbeddedServiceNames read the static baseline; drop the schema key-order machinery so envelope field order is alphabetical (JSON Schema property order is not semantic; parameterOrder for positional args is intact) - drop -tags larkmeta from Makefile, .goreleaser.yml, and build-pkg-pr-new.sh Command tree is byte-identical (8092 lines). registry/schema/cmd unit tests, the zero-alloc bench, and the e2e dry-run suite all pass.
The static-meta migration modeled only the method fields it explicitly declared, but the schema assembler still reads method["affordance"] to build _meta.affordance. metaschema.Method had no Affordance field and MethodToMap did not backfill it, so any method whose meta_data.json entry carries an affordance overlay would silently lose _meta.affordance in `schema --format json` (the embedded-JSON loader preserved it via the untyped map). Local meta currently has zero affordance entries, so no existing test caught it. Model affordance in the typed registry: - metaschema.Method gains Affordance plus Affordance/AffordanceExample types - gen.go emits the affordance literal (static, zero-alloc) - MethodToMap rebuilds method["affordance"] so parseAffordance keeps working schema JSON mode reads EmbeddedSpec (static baseline via ServiceToMap), which bypasses the remote overlay for determinism, so the remote/wire and MapToMethod paths -- never affordance consumers -- are left untouched. Add TestBuildMeta_AffordanceThroughTypedRegistry covering Method -> MethodToMap -> buildMeta -> _meta.affordance.
… doc typedInitialized was only assigned (in resetTyped) and never read, so staticcheck's U1000 (golangci `unused`) flagged it and would block the lint gate. Remove the field and its assignment. Also refresh the OrderedProps comment: the static-registry migration removed the meta_data.json natural-order machinery, so Order is now populated alphabetically (orderedKeys). The comment still claimed natural-order preservation. No behavior change.
… secret Building the command tree resolved brand (auth login --domain help) and strict mode (per-command identity-flag registration via ResolveStrictMode) through f.Config(), which decrypts the app secret. On macOS that is a Keychain (securityd IPC) read on every invocation -- even --help/--version/ schema/completion, which never use the secret. brand and strict mode are plain config.json fields, so route them through a secret-free metadata path: - credential: add (*DefaultAccountProvider).ResolveMeta and a cached (*CredentialProvider).ResolveMeta returning brand + SupportedIdentities, via an optional metaResolver capability; external providers fall back to ResolveAccount (they do not touch the keychain). - cmdutil: ResolveStrictMode now uses ResolveMeta; add Factory.ConfigBrand. - auth login help text and shortcut registration read ConfigBrand. The app secret is still decrypted on demand when a command actually calls the API (auth status etc. unchanged). Measured on macOS (GOMAXPROCS=1): lark-cli --help 35.4ms -> 21.4ms (-40%); no behavior change on Linux/Windows (local secret backends are cheap).
📝 WalkthroughWalkthroughThis PR migrates the service registry from untyped ChangesStatic Registry and Typed Metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1387 +/- ##
==========================================
+ Coverage 71.47% 71.86% +0.38%
==========================================
Files 688 700 +12
Lines 65482 66406 +924
==========================================
+ Hits 46806 47724 +918
+ Misses 15031 14997 -34
- Partials 3645 3685 +40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3b6aa7dc6a74df7537aa20eed57b9e77d1c0bdc9🧩 Skill updatenpx skills add larksuite/cli#feat/startup-perf -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: 2
🧹 Nitpick comments (2)
cmd/command_tree_dump_test.go (1)
71-87: HandleLARK_TREE_DUMPoutput safely (vfs +validate.SafeOutputPath) inTestDumpCommandTree
cmd/command_tree_dump_test.gowritesos.WriteFile(out, ...)whereout := os.Getenv("LARK_TREE_DUMP"), with novalidate.SafeOutputPath(and usesos.*instead ofvfs.*).- This matches the existing “manual debug output” pattern in
shortcuts/register_test.go(TestGenerateShortcutsJSONusesos.Getenv("SHORTCUTS_OUTPUT")+os.WriteFilewithoutvalidate.SafeOutputPath).Decide whether
TestDumpCommandTreeshould follow the stricter safety approach used for--outputpaths elsewhere (e.g.,validate.SafeOutputPath+vfs.*), or explicitly treat these env-controlled dump/tests as an exemption for now.🤖 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/command_tree_dump_test.go` around lines 71 - 87, The test TestDumpCommandTree currently writes the environment-controlled path directly with os.WriteFile, which bypasses path safety checks and the virtual FS; change it to validate the output path with validate.SafeOutputPath(out) and write using the project's vfs utilities (e.g., vfs.WriteFile or the repo's vfs API) instead of os.WriteFile so the test uses the same safe-output and VFS semantics as other --output code paths; update imports accordingly and keep the rest of TestDumpCommandTree and dumpCommandTree unchanged.Source: Coding guidelines
internal/registry/metastatic_validate_test.go (1)
85-90: ⚡ Quick winSkip benchmark when static registry is not generated.
Unlike the tests, the benchmark doesn’t guard the bare-checkout case. Adding a skip keeps benchmark numbers meaningful.
Suggested patch
func BenchmarkReadStaticRegistry(b *testing.B) { + if len(metastatic.Registry.Services) == 0 { + b.Skip("static registry empty; run `make fetch_meta` to generate it") + } b.ReportAllocs() for i := 0; i < b.N; i++ { sinkInt = deepReadStatic() } }🤖 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/registry/metastatic_validate_test.go` around lines 85 - 90, BenchmarkReadStaticRegistry must skip when the static registry isn't generated; add a pre-check at the top of BenchmarkReadStaticRegistry that detects whether the generated static registry exists and calls b.Skip with a clear message if it does not. Implement a small helper (e.g. staticRegistryGenerated or ensureStaticRegistryGenerated) that does the existence check (file/marker or other project-specific indicator) and returns a bool or calls b.Skip, then use it before invoking deepReadStatic so the benchmark is only run when the static registry is present.
🤖 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 `@internal/registry/metastatic/stub.go`:
- Line 12: Update the top-of-file stub comment in
internal/registry/metastatic/stub.go to reference the current generation command
used by the build flow: replace the outdated "make gen_meta" text with "make
fetch_meta" so contributors running the project on fresh checkouts use the
correct command.
In `@internal/registry/typed.go`:
- Around line 97-126: The function typedServiceNames has a race: it releases
typedMu.RLock() before iterating remoteOverrides, allowing setRemoteOverrides to
mutate it; fix by keeping the RLock for the entire read/snapshot phase — acquire
typedMu.RLock(), check typedNamesCache, compute seen by iterating
baselineServices() and remoteOverrides while still holding the RLock, build the
names slice (or copy map keys) under the RLock, then RUnlock(), acquire
typedMu.Lock(), re-check typedNamesCache (in case it was populated meanwhile)
and only then set typedNamesCache = names and Unlock(); refer to
typedServiceNames, typedMu, typedNamesCache, remoteOverrides and
setRemoteOverrides when making the change.
---
Nitpick comments:
In `@cmd/command_tree_dump_test.go`:
- Around line 71-87: The test TestDumpCommandTree currently writes the
environment-controlled path directly with os.WriteFile, which bypasses path
safety checks and the virtual FS; change it to validate the output path with
validate.SafeOutputPath(out) and write using the project's vfs utilities (e.g.,
vfs.WriteFile or the repo's vfs API) instead of os.WriteFile so the test uses
the same safe-output and VFS semantics as other --output code paths; update
imports accordingly and keep the rest of TestDumpCommandTree and dumpCommandTree
unchanged.
In `@internal/registry/metastatic_validate_test.go`:
- Around line 85-90: BenchmarkReadStaticRegistry must skip when the static
registry isn't generated; add a pre-check at the top of
BenchmarkReadStaticRegistry that detects whether the generated static registry
exists and calls b.Skip with a clear message if it does not. Implement a small
helper (e.g. staticRegistryGenerated or ensureStaticRegistryGenerated) that does
the existence check (file/marker or other project-specific indicator) and
returns a bool or calls b.Skip, then use it before invoking deepReadStatic so
the benchmark is only run when the static registry is present.
🪄 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: 6236f1e2-6fd2-4282-9ff3-d4c4d605c5d6
📒 Files selected for processing (29)
.gitignore.goreleaser.ymlMakefilecmd/auth/login.gocmd/command_tree_dump_test.gocmd/service/service.gocmd/service/service_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/testing.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/meta_data_default.jsoninternal/registry/metaschema/types.gointernal/registry/metastatic/gen.gointernal/registry/metastatic/stub.gointernal/registry/metastatic_validate_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/typed.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/types.goscripts/build-pkg-pr-new.shscripts/fetch_meta.pyshortcuts/register.goshortcuts/register_brand_guard_test.go
💤 Files with no reviewable changes (2)
- internal/registry/meta_data_default.json
- internal/registry/loader_embedded.go
| // value) so the package always compiles, and populated by meta_data_gen.go's | ||
| // init() when that generated file is present. On a fresh checkout the generated | ||
| // file is absent — it is gitignored and produced at build time by | ||
| // `make gen_meta` — so Registry stays empty. This keeps the "heavy spec is |
There was a problem hiding this comment.
Use the current generation command in the stub comment.
The comment references make gen_meta, but the project’s build flow uses make fetch_meta. This can mislead contributors on fresh checkouts.
Suggested patch
-// `make gen_meta` — so Registry stays empty. This keeps the "heavy spec is
+// `make fetch_meta` — so Registry stays empty. This keeps the "heavy spec is📝 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.
Suggested change
| // `make gen_meta` — so Registry stays empty. This keeps the "heavy spec is | |
| // `make fetch_meta` — so Registry stays empty. This keeps the "heavy spec is |
🤖 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/registry/metastatic/stub.go` at line 12, Update the top-of-file stub
comment in internal/registry/metastatic/stub.go to reference the current
generation command used by the build flow: replace the outdated "make gen_meta"
text with "make fetch_meta" so contributors running the project on fresh
checkouts use the correct command.
Comment on lines
+97
to
+126
| func typedServiceNames() []string { | ||
| typedMu.RLock() | ||
| if typedNamesCache != nil { | ||
| out := typedNamesCache | ||
| typedMu.RUnlock() | ||
| return out | ||
| } | ||
| typedMu.RUnlock() | ||
|
|
||
| seen := make(map[string]bool) | ||
| for _, s := range baselineServices() { | ||
| seen[s.Name] = true | ||
| } | ||
| typedMu.RLock() | ||
| for name := range remoteOverrides { | ||
| seen[name] = true | ||
| } | ||
| typedMu.RUnlock() | ||
|
|
||
| names := make([]string, 0, len(seen)) | ||
| for n := range seen { | ||
| names = append(names, n) | ||
| } | ||
| sort.Strings(names) | ||
|
|
||
| typedMu.Lock() | ||
| typedNamesCache = names | ||
| typedMu.Unlock() | ||
| return names | ||
| } |
There was a problem hiding this comment.
Race window between cache check and overlay iteration.
Between releasing RLock at line 104 and re-acquiring it at line 110, setRemoteOverrides can run on another goroutine, modifying remoteOverrides. This creates a window where:
- Cache is nil → release lock
- Another goroutine calls
setRemoteOverrides, adding/modifying services - This goroutine iterates
remoteOverrideswith partial/stale view
The result is a names list that may be inconsistent with the actual overlay state.
🔒 Suggested fix: hold RLock through the entire computation
func typedServiceNames() []string {
typedMu.RLock()
if typedNamesCache != nil {
out := typedNamesCache
typedMu.RUnlock()
return out
}
- typedMu.RUnlock()
seen := make(map[string]bool)
for _, s := range baselineServices() {
seen[s.Name] = true
}
- typedMu.RLock()
for name := range remoteOverrides {
seen[name] = true
}
typedMu.RUnlock()
names := make([]string, 0, len(seen))
for n := range seen {
names = append(names, n)
}
sort.Strings(names)
typedMu.Lock()
+ // Double-check: another goroutine may have populated the cache
+ if typedNamesCache != nil {
+ typedMu.Unlock()
+ return typedNamesCache
+ }
typedNamesCache = names
typedMu.Unlock()
return names
}🤖 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/registry/typed.go` around lines 97 - 126, The function
typedServiceNames has a race: it releases typedMu.RLock() before iterating
remoteOverrides, allowing setRemoteOverrides to mutate it; fix by keeping the
RLock for the entire read/snapshot phase — acquire typedMu.RLock(), check
typedNamesCache, compute seen by iterating baselineServices() and
remoteOverrides while still holding the RLock, build the names slice (or copy
map keys) under the RLock, then RUnlock(), acquire typedMu.Lock(), re-check
typedNamesCache (in case it was populated meanwhile) and only then set
typedNamesCache = names and Unlock(); refer to typedServiceNames, typedMu,
typedNamesCache, remoteOverrides and setRemoteOverrides when making the change.
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
Speeds up lark-cli cold start on all three platforms by removing two per-invocation startup costs:
meta_data.jsonintomap[string]interface{}on every run, andChanges
internal/registry/metastatic, produced byscripts/fetch_meta.py) instead of parsing JSON at startup. Command tree is byte-identical to the old parse path (verified viacmd/command_tree_dump_test.go); the embedded baseline reads with zero heap allocation; the remote overlay is still decoded at runtime.meta_data.jsonis no longer embedded.metaschema.Methodnow carries theaffordanceoverlay, soschema --format jsonkeeps emitting_meta.affordance(the old untyped map preserved it implicitly).ResolveMetapath, so building the command tree no longer decrypts the app secret. On macOS this removes a Keychain (securityd IPC) read on every invocation. The secret is still decrypted on demand when a command actually calls the API (auth statusetc. unchanged).Behavior change
schema --format jsonobject property order is now alphabetical (the natural-order machinery tied to rawmeta_data.jsonbytes was removed). JSON Schema / MCP tool-spec property order is semantically insignificant;--format prettyis unaffected. Asserted inassembler_test.go.Test plan
go test ./...green (registry, schema, service, cmdutil, credential, shortcuts).TestStaticReadZeroAlloc).Summary by CodeRabbit
Release Notes
Performance
Refactor