Skip to content

feat(allowlist): add Thrift and Cap'n Proto support - #960

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
chethanuk:wf/thrift-capnp-allowlist
Aug 18, 2026
Merged

feat(allowlist): add Thrift and Cap'n Proto support#960
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
chethanuk:wf/thrift-capnp-allowlist

Conversation

@chethanuk

Copy link
Copy Markdown
Contributor

Description

.thrift and .capnp files were never reviewed. Neither extension is in supported_file_types.json, so IsAllowedExt drops them at the filter gate before any rule can run.

This adds both to the allowlist and registers **/*.thriftthrift.md and **/*.capnpcapnp.md in path_rule_map. Two docs rather than one shared IDL doc, because almost nothing carries over between them. Thrift breaks when a required field is added to a live struct, when a retired field id is reused (there is no reserved keyword to hold it open), and when a service method is renamed, since method names travel on the wire. Cap'n Proto breaks when an @N ordinal moves, when a UInt32 is widened to UInt64 (fixed-width slots, unlike a protobuf varint), and when a struct is renamed with no explicit @0x... id pinned, since the id is derived from the name. A merged doc would spend half its lines telling the model which half to ignore. Data only, no Go source changes.

The generated-output excludes are scoped by extension (**/gen-{cpp,cpp2,go,py,...}/**/*.{c,cc,go,py,...}, not a bare **/gen-*/**). IsExcludedPath applies every pattern to every path with no language dispatch, so an unscoped directory glob would drop unrelated files in other languages that happen to sit under a colliding directory name. That is why the file already carries **/Tests/**/*.swift and **/tests/**/*.nim rather than the directory alone. Three negative rows in TestIsExcludedPath fail if anyone broadens it later.

Only the Cap'n Proto C++ header gets a pattern. The generator also emits .capnp.c++, but .c++ was never allowlisted, so those files are already gone by the time IsExcludedPath runs.

Limitation

.thrift and .capnp diffs that used to be skipped are now reviewed. Anyone who does not want that opts out with user exclude globs, which are checked before the allowlist.

Type of Change

  • New feature (non-breaking change that adds functionality)

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Rows appended to the existing table tests:

  • TestIsAllowedExt.thrift and .capnp, both cases.
  • TestIsExcludedPath — the gen-*, kitex_gen and Cap'n Proto output patterns, plus negatives (tools/gen-go/config.yaml, site/gen-js/index.html, kitex_gen/api/schema.json) that fail if the patterns lose their extension scope, and idl/service.thrift / schema/addressbook.capnp to pin that the schemas themselves stay in review.
  • TestResolve_DefaultRules.thrift and .capnp paths resolve by heading anchors unique to each doc.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

The rule table in pages/src/content/docs/{en,ja,ru,zh}/review-rules.md lists both new patterns.

Related Issues

Part of #470.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

@chethanuk
chethanuk force-pushed the wf/thrift-capnp-allowlist branch from 66d122a to 2b0c847 Compare August 17, 2026 06:21
@chethanuk

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Please review and merge this. Future main updates might cause more conflicts. Thanks :)

@chethanuk
chethanuk force-pushed the wf/thrift-capnp-allowlist branch from 2b0c847 to b99c62c Compare August 17, 2026 07:35
@chethanuk
chethanuk force-pushed the wf/thrift-capnp-allowlist branch from b99c62c to 4e57d59 Compare August 17, 2026 12:30
@chethanuk
chethanuk force-pushed the wf/thrift-capnp-allowlist branch from 4e57d59 to 709246c Compare August 18, 2026 08:50
@chethanuk

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Please review and merge this. Future main updates might cause more conflicts. I resolved conflicts 3 times now... Thanks :)

"**/test/**/*.zig",
"**/*_test.zig"
"**/*_test.zig",
"**/gen-{cpp,cpp2,c_glib,cocoa,csharp,netstd,dart,erl,go,hs,hs2,java,js,nodejs,kotlin,lua,perl,php,py,py3,rb,rs,swift}/**/*.{c,cc,cpp,cs,dart,erl,go,h,hrl,hs,java,js,kt,lua,m,mm,php,pl,pm,py,pyi,rb,rs,swift,ts}",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chethanuk I feel that the hit rate for this matching rule is quite low and it is prone to accidental deletion; could we remove it?

Suggested change
"**/gen-{cpp,cpp2,c_glib,cocoa,csharp,netstd,dart,erl,go,hs,hs2,java,js,nodejs,kotlin,lua,perl,php,py,py3,rb,rs,swift}/**/*.{c,cc,cpp,cs,dart,erl,go,h,hrl,hs,java,js,kt,lua,m,mm,php,pl,pm,py,pyi,rb,rs,swift,ts}",

Thrift IDL (.thrift) and Cap'n Proto schema (.capnp) files were dropped at
the extension gate, so no rule could ever run on them. Both are IDLs whose
main review risk is silent wire-compatibility breakage, which is exactly
what protobuf.md already covers for .proto.

Adds both extensions to the allowlist, registers thrift.md and capnp.md in
path_rule_map, and excludes the compilers' generated output. The gen-* and
kitex_gen excludes are scoped by extension rather than by directory because
IsExcludedPath applies every pattern to every path with no language
dispatch; a bare gen-*/** would drop unrelated files in other languages.
Data only, no Go source changes.
@chethanuk
chethanuk force-pushed the wf/thrift-capnp-allowlist branch from 709246c to 7f6787d Compare August 18, 2026 09:53

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 6c4a89f into alibaba:main Aug 18, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants