refactor: converge command pipelines onto a typed metadata model + catalog#1191
Open
liangshuo-1 wants to merge 2 commits into
Open
refactor: converge command pipelines onto a typed metadata model + catalog#1191liangshuo-1 wants to merge 2 commits into
liangshuo-1 wants to merge 2 commits into
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplace untyped map-based API metadata with typed internal/meta models and add internal/apicatalog; migrate registry loaders/caching, schema envelope assembly, service CLI wiring (flags/help/affordance), scope resolution/completion, and tests to the typed/catalog-driven flow. ChangesTyped Metadata & Catalog
Registry & caching
Schema assembler & types
Service CLI
Schema CLI & error hints
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1191 +/- ##
==========================================
+ Coverage 69.14% 69.60% +0.46%
==========================================
Files 630 641 +11
Lines 59304 58914 -390
==========================================
+ Hits 41005 41009 +4
+ Misses 14988 14675 -313
+ Partials 3311 3230 -81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@55eb64a6571ee9bad9c39e2e606f688d40ad2919🧩 Skill updatenpx skills add larksuite/cli#feat/typed-meta-model -y -g |
liangshuo-1
force-pushed
the
feat/typed-meta-model
branch
from
a596d0f to
bc06233
Compare
liangshuo-1
changed the title
![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)
internal/registry/scopes.go (1)
126-154: 💤 Low valuePotential data race on
cachedAllScopesmap.The cache initialization and access pattern is not thread-safe. If
CollectAllScopesFromMetais called concurrently from multiple goroutines, the nil check, map creation, and writes can race. While CLI commands typically run single-threaded, this could cause issues if the function is used in concurrent contexts (e.g., tests witht.Parallel()).Consider using
sync.Onceorsync.Mapfor thread-safe caching, similar to howembeddedParseOnceis used inloader.go.♻️ Proposed fix using sync.Once per identity
var cachedAllScopes map[string][]string +var cachedAllScopesMu sync.Mutex // CollectAllScopesFromMeta collects all unique scopes from from_meta/*.json // for the given identity ("user" or "tenant"). Results are deduplicated and sorted. func CollectAllScopesFromMeta(identity string) []string { + cachedAllScopesMu.Lock() if cachedAllScopes == nil { cachedAllScopes = make(map[string][]string) } if cached, ok := cachedAllScopes[identity]; ok { + cachedAllScopesMu.Unlock() return cached } + cachedAllScopesMu.Unlock() wantToken := meta.TokenForIdentity(identity) supported := func(m meta.Method) bool { return m.SupportsToken(wantToken) } scopeSet := make(map[string]bool) for _, ref := range RuntimeCatalog().WalkMethods(supported) { for _, s := range ref.Method.Scopes { scopeSet[s] = true } } result := make([]string, 0, len(scopeSet)) for s := range scopeSet { result = append(result, s) } sort.Strings(result) + cachedAllScopesMu.Lock() cachedAllScopes[identity] = result + cachedAllScopesMu.Unlock() return result }🤖 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/scopes.go` around lines 126 - 154, CollectAllScopesFromMeta uses the package-level map cachedAllScopes without synchronization which can race under concurrent calls; protect access by adding a package-level mutex (e.g., cachedAllScopesMu sync.RWMutex) and wrap reads/writes: use RLock/RUnlock to check for an existing cached value, and if not present upgrade to Lock/Unlock to create the map if nil, compute the scopes, and assign cachedAllScopes[identity] = result; ensure the double-check pattern (check under RLock, recheck under Lock) so you don't recompute concurrently. Reference: CollectAllScopesFromMeta and cachedAllScopes.cmd/schema/schema.go (1)
106-130: ⚡ Quick winClarify
resolveErrorfall-through reachability incmd/schema/schema.goThe raw
return errfall-throughs are currently unreachable:internal/apicatalog.Catalog.Resolvereturns only*apicatalog.ResolveErroron failure, and itsResolveError.Kindis limited toapicatalog.ErrService,apicatalog.ErrResource,apicatalog.ErrMethod, andapicatalog.ErrPath, all of which are handled.
Optional: add adefaultcase (or wrap the fall-throughs) to preserve the stderr JSON contract if new kinds/extra error types are introduced later.🤖 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/schema/schema.go` around lines 106 - 130, The resolveError function currently switches on apicatalog.ResolveError.Kind but handles all known kinds, making the final return err unreachable; add a default switch case (or an else after errors.As) that returns the original err (or wraps it consistently via output.ErrWithHint/output.ExitValidation) to preserve the stderr JSON contract if new ResolveError kinds or other error types appear later—update the switch in resolveError (and the errors.As branch) to include a default branch that returns the unmodified error or a consistent error wrapper.
🤖 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/remote.go`:
- Around line 41-44: The cache round-trip for meta_data.json can drop unknown
JSON keys because MergedRegistry uses typed meta.Service/meta.Method/meta.Field
which only declare explicit json fields; to fix, preserve unknown keys by either
adding a catch-all raw storage field to those types (e.g., an Extra
map[string]json.RawMessage or json.RawMessage wrapper) or by storing and reusing
the original raw JSON for each service when creating MergedRegistry; update the
definitions in meta.Service/meta.Method/meta.Field (or change MergedRegistry to
carry raw payloads) so the cache write emits the original keys unchanged and use
those raw values when marshalling the cached meta_data.json.
---
Nitpick comments:
In `@cmd/schema/schema.go`:
- Around line 106-130: The resolveError function currently switches on
apicatalog.ResolveError.Kind but handles all known kinds, making the final
return err unreachable; add a default switch case (or an else after errors.As)
that returns the original err (or wraps it consistently via
output.ErrWithHint/output.ExitValidation) to preserve the stderr JSON contract
if new ResolveError kinds or other error types appear later—update the switch in
resolveError (and the errors.As branch) to include a default branch that returns
the unmodified error or a consistent error wrapper.
In `@internal/registry/scopes.go`:
- Around line 126-154: CollectAllScopesFromMeta uses the package-level map
cachedAllScopes without synchronization which can race under concurrent calls;
protect access by adding a package-level mutex (e.g., cachedAllScopesMu
sync.RWMutex) and wrap reads/writes: use RLock/RUnlock to check for an existing
cached value, and if not present upgrade to Lock/Unlock to create the map if
nil, compute the scopes, and assign cachedAllScopes[identity] = result; ensure
the double-check pattern (check under RLock, recheck under Lock) so you don't
recompute concurrently. Reference: CollectAllScopesFromMeta and cachedAllScopes.
🪄 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: 320d51ee-3b09-4905-8c0d-bfe2ae6d1bf6
📒 Files selected for processing (50)
cmd/auth/login_interactive.gocmd/command_catalog_path_test.gocmd/error_auth_hint.gocmd/root.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups.gocmd/service/flaggroups_test.gocmd/service/paramflags.gocmd/service/paramflags_test.gocmd/service/sanitize_test.gocmd/service/service.gocmd/service/service_risk_test.gocmd/service/service_test.gointernal/apicatalog/catalog.gointernal/apicatalog/catalog_test.gointernal/apicatalog/methodref.gointernal/apicatalog/path.gointernal/apicatalog/path_test.gointernal/apicatalog/resolveerror.gointernal/cmdutil/fileupload.gointernal/cmdutil/identity.gointernal/cmdutil/identity_test.gointernal/cmdutil/risk.gointernal/core/risk.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/meta/identity.gointernal/meta/identity_test.gointernal/meta/meta.gointernal/meta/meta_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/registry/catalog.gointernal/registry/helpers.gointernal/registry/loader.gointernal/registry/registry_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint.gointernal/registry/scopes.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/path.gointernal/schema/types.gointernal/schema/types_test.go
💤 Files with no reviewable changes (4)
- internal/schema/path.go
- internal/cmdutil/identity_test.go
- internal/cmdutil/identity.go
- internal/cmdutil/fileupload.go
liangshuo-1
force-pushed
the
feat/typed-meta-model
branch
2 times, most recently
from
830b07c to
7af9820
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/registry/remote.go (1)
38-44:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve unknown remote metadata fields in the cache.
This now decodes
servicesinto[]meta.Serviceand then re-marshalsenvelope.Databefore writingremote_meta.json. Any fields the typed model does not declare are lost on that round-trip, so the cache can silently diverge from the server payload and hide newly shipped metadata until the binary is updated. Please cache the rawdataJSON or carry unknown fields through the typed model before writing.Also applies to: 219-225
🤖 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/remote.go` around lines 38 - 44, The cache currently decodes envelope.Data into []meta.Service (type MergedRegistry with Services []meta.Service) which drops unknown fields when re-marshaling remote_meta.json; preserve unknown remote metadata by either storing the raw JSON (e.g. keep envelope.Data as a json.RawMessage and write that into remote_meta.json) or by changing the MergedRegistry.Services field to hold the raw service objects (e.g. []json.RawMessage or []map[string]interface{}) and only decode into meta.Service where typed access is needed; update the code paths that build/write remote_meta.json (the code handling envelope.Data and MergedRegistry) to write the preserved raw JSON instead of the re-marshaled typed struct so unknown fields are kept.
🤖 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/meta/affordance_test.go`:
- Around line 13-25: The test currently uses an empty json.RawMessage for the
"nil" case but doesn't exercise a true nil payload; update the test to add a
separate case that passes a nil Affordance (e.g., Method{Affordance:
json.RawMessage(nil)} or a zero-value Affordance) and assert ParsedAffordance()
returns ok=false, while leaving the existing `""` entry to continue testing an
empty payload; reference the Method type and its ParsedAffordance() method when
making the change.
---
Duplicate comments:
In `@internal/registry/remote.go`:
- Around line 38-44: The cache currently decodes envelope.Data into
[]meta.Service (type MergedRegistry with Services []meta.Service) which drops
unknown fields when re-marshaling remote_meta.json; preserve unknown remote
metadata by either storing the raw JSON (e.g. keep envelope.Data as a
json.RawMessage and write that into remote_meta.json) or by changing the
MergedRegistry.Services field to hold the raw service objects (e.g.
[]json.RawMessage or []map[string]interface{}) and only decode into meta.Service
where typed access is needed; update the code paths that build/write
remote_meta.json (the code handling envelope.Data and MergedRegistry) to write
the preserved raw JSON instead of the re-marshaled typed struct so unknown
fields are kept.
🪄 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: 54ab893f-ac27-4eef-b4eb-67737c9d44f8
📒 Files selected for processing (50)
cmd/auth/login_interactive.gocmd/command_catalog_path_test.gocmd/error_auth_hint.gocmd/root.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups.gocmd/service/flaggroups_test.gocmd/service/paramflags.gocmd/service/paramflags_test.gocmd/service/sanitize_test.gocmd/service/service.gocmd/service/service_risk_test.gocmd/service/service_test.gointernal/apicatalog/catalog.gointernal/apicatalog/catalog_test.gointernal/apicatalog/methodref.gointernal/apicatalog/path.gointernal/apicatalog/path_test.gointernal/apicatalog/resolveerror.gointernal/cmdutil/fileupload.gointernal/cmdutil/identity.gointernal/cmdutil/identity_test.gointernal/cmdutil/risk.gointernal/core/risk.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/meta/identity.gointernal/meta/identity_test.gointernal/meta/meta.gointernal/meta/meta_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/registry/catalog.gointernal/registry/helpers.gointernal/registry/loader.gointernal/registry/registry_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint.gointernal/registry/scopes.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/path.gointernal/schema/types.gointernal/schema/types_test.go
💤 Files with no reviewable changes (4)
- internal/schema/path.go
- internal/cmdutil/identity.go
- internal/cmdutil/identity_test.go
- internal/cmdutil/fileupload.go
🚧 Files skipped from review as they are similar to previous changes (35)
- internal/core/risk.go
- internal/schema/types_test.go
- cmd/service/affordance_test.go
- internal/cmdutil/risk.go
- internal/meta/affordance.go
- internal/apicatalog/resolveerror.go
- cmd/service/sanitize_test.go
- internal/apicatalog/methodref.go
- cmd/service/service_risk_test.go
- cmd/auth/login_interactive.go
- cmd/service/affordance.go
- internal/apicatalog/path.go
- cmd/command_catalog_path_test.go
- internal/meta/normalize_test.go
- cmd/service/flaggroups_test.go
- internal/apicatalog/path_test.go
- internal/registry/helpers.go
- internal/registry/remote_test.go
- internal/meta/identity_test.go
- internal/schema/lint.go
- internal/schema/assembler.go
- cmd/service/paramflags.go
- cmd/service/flaggroups.go
- internal/registry/registry_test.go
- internal/registry/scopes.go
- internal/apicatalog/catalog_test.go
- internal/meta/meta.go
- internal/schema/lint_test.go
- internal/meta/normalize.go
- cmd/error_auth_hint.go
- internal/registry/loader.go
- cmd/schema/schema_test.go
- cmd/service/paramflags_test.go
- cmd/service/service.go
- internal/schema/assembler_test.go
Comment on lines
+13
to
+25
| notOK := map[string]string{ | ||
| "nil": ``, | ||
| "empty object": `{}`, | ||
| "all empty arrays": `{"use_when":[],"do_not_use_when":[],"prerequisites":[],"examples":[],"related":[]}`, | ||
| "malformed string": `"not an object"`, | ||
| "malformed number": `42`, | ||
| "nested type mismatch": `{"examples":"should be a list"}`, | ||
| } | ||
| for name, raw := range notOK { | ||
| t.Run(name, func(t *testing.T) { | ||
| if _, ok := (Method{Affordance: json.RawMessage(raw)}).ParsedAffordance(); ok { | ||
| t.Errorf("ParsedAffordance(%s) ok=true, want false", raw) | ||
| } |
There was a problem hiding this comment.
Actually cover a nil affordance payload.
The "nil" case currently passes an empty json.RawMessage, not a nil one, so a regression in the nil-input branch would still pass. Add a separate zero-value / nil Affordance case and keep "" as the empty-payload case.
Suggested test adjustment
func TestMethod_ParsedAffordance(t *testing.T) {
// absent / empty / malformed all resolve to ok=false.
+ t.Run("nil", func(t *testing.T) {
+ if _, ok := (Method{}).ParsedAffordance(); ok {
+ t.Error("ParsedAffordance(nil) ok=true, want false")
+ }
+ })
+
notOK := map[string]string{
- "nil": ``,
+ "empty payload": ``,
"empty object": `{}`,
"all empty arrays": `{"use_when":[],"do_not_use_when":[],"prerequisites":[],"examples":[],"related":[]}`,
"malformed string": `"not an object"`,
"malformed number": `42`,
"nested type mismatch": `{"examples":"should be a list"}`,🤖 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 13 - 25, The test currently
uses an empty json.RawMessage for the "nil" case but doesn't exercise a true nil
payload; update the test to add a separate case that passes a nil Affordance
(e.g., Method{Affordance: json.RawMessage(nil)} or a zero-value Affordance) and
assert ParsedAffordance() returns ok=false, while leaving the existing `""`
entry to continue testing an empty payload; reference the Method type and its
ParsedAffordance() method when making the change.
5401796 to
40dda0d
Compare
…talog - New internal/meta: typed model over meta_data.json; owns field normalization + the access-token<->identity bijection (3 duplicate token interpreters merged into one). - New internal/apicatalog: navigation (Resolve/MethodRefs/WalkMethods/ ServiceMethods/Complete); path resolution moved out of internal/schema. - internal/schema is render-only; risk constants moved to internal/core. - internal/registry keeps source + scope policy and walks the catalog; cmd/service param flags extracted to paramflags.go/flaggroups.go. - Add: typed flags for raw API commands; grouped --help with inline enum value=meaning + method affordance; envelope enumDescriptions; schema tolerates --format/--json for agent compatibility (always emits the JSON envelope). - Fix: tolerant Option.Value so one bad value can't blank the catalog; nested-resource completion; auth-hint nested path. - Remove: byte-for-byte envelope golden. Change-Id: Ia7b870b3d4c6f2d56b9dc3dcdc91f2fdaeaddae4
liangshuo-1
force-pushed
the
feat/typed-meta-model
branch
from
40dda0d to
1739a1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/meta/meta.go (1)
195-215: ⚡ Quick winConsider panicking on marshal/unmarshal errors in test helpers.
FromMapandServiceFromMapsilently return zero values when JSON encoding fails. Since these are test-only helpers (per the comments), silent failures can hide malformed test fixtures.Consider:
func FromMap(method map[string]interface{}) Method { b, err := json.Marshal(method) if err != nil { - return Method{} + panic(fmt.Sprintf("FromMap: marshal failed: %v", err)) } var m Method - _ = json.Unmarshal(b, &m) + if err := json.Unmarshal(b, &m); err != nil { + panic(fmt.Sprintf("FromMap: unmarshal failed: %v", err)) + } return m }🤖 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/meta.go` around lines 195 - 215, The FromMap and ServiceFromMap test helpers currently swallow json.Marshal/json.Unmarshal errors and return zero values; change them to fail fast by panicking on marshal or unmarshal errors so malformed test fixtures are obvious: in FromMap (function FromMap, type Method) and ServiceFromMap (function ServiceFromMap, type Service) check errs from json.Marshal and json.Unmarshal and call panic(fmt.Errorf(...)) with the error (or use panic(err)) instead of returning an empty struct.
🤖 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/service/paramflags.go`:
- Around line 72-81: In paramFlagBinder.overlay, guard against params being nil
to avoid a panic when params was parsed from a JSON "null": check params at the
start of overlay (or before writing) and either return early or allocate a new
map[string]interface{} so that assignments like params[pf.field.Name] =
pf.read() are safe; update the function containing overlay
(paramFlagBinder.overlay) to perform this nil-check/initialization before the
loop that uses pf.field.Name and pf.read().
---
Nitpick comments:
In `@internal/meta/meta.go`:
- Around line 195-215: The FromMap and ServiceFromMap test helpers currently
swallow json.Marshal/json.Unmarshal errors and return zero values; change them
to fail fast by panicking on marshal or unmarshal errors so malformed test
fixtures are obvious: in FromMap (function FromMap, type Method) and
ServiceFromMap (function ServiceFromMap, type Service) check errs from
json.Marshal and json.Unmarshal and call panic(fmt.Errorf(...)) with the error
(or use panic(err)) instead of returning an empty struct.
🪄 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: 79fd8003-ccee-4fa1-a39d-0bc311342291
📒 Files selected for processing (50)
cmd/auth/login_interactive.gocmd/command_catalog_path_test.gocmd/error_auth_hint.gocmd/root.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/flaggroups.gocmd/service/flaggroups_test.gocmd/service/paramflags.gocmd/service/paramflags_test.gocmd/service/sanitize_test.gocmd/service/service.gocmd/service/service_risk_test.gocmd/service/service_test.gointernal/apicatalog/catalog.gointernal/apicatalog/catalog_test.gointernal/apicatalog/methodref.gointernal/apicatalog/path.gointernal/apicatalog/path_test.gointernal/apicatalog/resolveerror.gointernal/cmdutil/fileupload.gointernal/cmdutil/identity.gointernal/cmdutil/identity_test.gointernal/cmdutil/risk.gointernal/core/risk.gointernal/meta/affordance.gointernal/meta/affordance_test.gointernal/meta/identity.gointernal/meta/identity_test.gointernal/meta/meta.gointernal/meta/meta_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/registry/catalog.gointernal/registry/helpers.gointernal/registry/loader.gointernal/registry/registry_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint.gointernal/registry/scopes.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/path.gointernal/schema/types.gointernal/schema/types_test.go
💤 Files with no reviewable changes (4)
- internal/cmdutil/identity.go
- internal/cmdutil/identity_test.go
- internal/schema/path.go
- internal/cmdutil/fileupload.go
✅ Files skipped from review due to trivial changes (2)
- cmd/service/service_risk_test.go
- cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (30)
- internal/schema/types_test.go
- internal/meta/affordance_test.go
- internal/registry/catalog.go
- internal/schema/lint_test.go
- internal/core/risk.go
- internal/meta/affordance.go
- cmd/auth/login_interactive.go
- internal/apicatalog/path.go
- internal/meta/meta_test.go
- internal/cmdutil/risk.go
- cmd/service/sanitize_test.go
- cmd/service/affordance.go
- cmd/service/affordance_test.go
- cmd/service/flaggroups_test.go
- internal/schema/lint.go
- internal/meta/identity_test.go
- internal/meta/identity.go
- internal/meta/normalize_test.go
- cmd/command_catalog_path_test.go
- cmd/schema/schema_test.go
- cmd/error_auth_hint.go
- internal/meta/normalize.go
- cmd/service/paramflags_test.go
- internal/registry/registry_test.go
- internal/apicatalog/catalog_test.go
- internal/registry/scopes.go
- internal/registry/remote_test.go
- internal/registry/loader.go
- internal/schema/assembler.go
- internal/schema/assembler_test.go
The JSON literal `null` unmarshals into a nil map with err==nil, so
ParseJSONMap reported success while handing back an unwritable map. Both
callers then panic on the first overlay write:
- cmd/service: paramFlagBinder.overlay sets params[field] = flag value
- cmd/api: params["page_size"] = N
Triggered by e.g. `--params null --chat-id X` or `--params null --page-size N`.
Normalize a nil result to an empty map at the source, so the contract
("returns a writable map on success") holds for every caller and matches
the existing empty-input case. Regression tests pin the contract at the
unit level and at both call sites.
Change-Id: I75e695c039be96acfc24f4fd134a64a2984bd1e7
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.
Operation-runtime convergence (PR1).
Refactor
internal/meta— typed model overmeta_data.json; owns field normalization + access-token↔identity (3 duplicate token interpreters merged into one).internal/apicatalog— navigation (Resolve/MethodRefs/WalkMethods/ServiceMethods/Complete); path resolution moved out ofinternal/schema.internal/schemais render-only; risk constants moved tointernal/core.internal/registrykeeps source + scope policy and walks the catalog;cmd/service/cmd/schemathinned to adapters, param flags extracted toparamflags.go/flaggroups.go.Added
--helpwith inline enumvalue=meaning+ method affordance.enumDescriptions.schematolerates--format/--json(hidden, ignored) for agent compatibility — always emits the JSON envelope.Fixed
Option.Value(one bad value can't blank the catalog) + numeric enum sort.Removed
Summary by CodeRabbit
New Features
Bug Fixes
Refactor