From 7f6787ddbb098d05f567c919e2aad59f0b4d9a1b Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Mon, 17 Aug 2026 00:22:27 +0200 Subject: [PATCH] feat(allowlist): add Thrift and Cap'n Proto support 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. --- internal/config/allowlist/allowed_ext_test.go | 22 ++++++++++++ .../allowlist/default_exclude_patterns.json | 8 ++++- .../allowlist/supported_file_types.json | 4 ++- internal/config/rules/rule_docs/capnp.md | 36 +++++++++++++++++++ internal/config/rules/rule_docs/thrift.md | 35 ++++++++++++++++++ internal/config/rules/system_rules.json | 4 ++- internal/config/rules/system_rules_test.go | 4 +++ pages/src/content/docs/en/review-rules.md | 2 ++ pages/src/content/docs/ja/review-rules.md | 2 ++ pages/src/content/docs/ru/review-rules.md | 2 ++ pages/src/content/docs/zh/review-rules.md | 2 ++ 11 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 internal/config/rules/rule_docs/capnp.md create mode 100644 internal/config/rules/rule_docs/thrift.md diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index 83abb167c..d8e59d06e 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -79,6 +79,10 @@ func TestIsAllowedExt(t *testing.T) { {".LIBSONNET", true}, {".zig", true}, {".ZIG", true}, + {".thrift", true}, + {".THRIFT", true}, + {".capnp", true}, + {".CAPNP", true}, {".txt", false}, {".md", false}, {".png", false}, @@ -200,6 +204,24 @@ func TestIsExcludedPath(t *testing.T) { {"elm nested test directory", "packages/core/tests/unit/ParserTest.elm", true}, {"elm non-test", "src/Parser.elm", false}, {"elm tests in filename", "src/TestsHelper.elm", false}, + // Thrift generated output directories + {"kitex_gen at root", "kitex_gen/api/service.go", true}, + {"kitex_gen nested", "app/rpc/kitex_gen/user/user.go", true}, + {"thrift idl is reviewed", "idl/service.thrift", false}, + {"hand-written generated-ish dir name", "services/generated_client/client.go", false}, + {"gen in package name only", "internal/generator/main.go", false}, + // The kitex_gen pattern is extension-anchored: a colliding directory name + // must not drop files Thrift never emits (IsExcludedPath has no language dispatch). + {"kitex_gen holding non-Go file", "kitex_gen/api/schema.json", false}, + + // Cap'n Proto generated output files + {"capnp generated header", "src/schema.capnp.h", true}, + {"capnp generated go", "tunnelrpc/proto/tunnelrpc.capnp.go", true}, + {"capnp generated rust", "src/element_capnp.rs", true}, + {"capnp generated typescript", "src/rpc.capnp.ts", true}, + {"capnp generated python", "schema/addressbook_capnp.py", true}, + {"capnp schema is reviewed", "schema/addressbook.capnp", false}, + {"capnp in filename only", "src/capnp_helpers.go", false}, // Jsonnet vendored dependencies (written by `jb install`, wiped by `rm -rf vendor`). // The pattern is extension-scoped: IsExcludedPath applies every pattern to every diff --git a/internal/config/allowlist/default_exclude_patterns.json b/internal/config/allowlist/default_exclude_patterns.json index 9f05b470b..b88d85086 100644 --- a/internal/config/allowlist/default_exclude_patterns.json +++ b/internal/config/allowlist/default_exclude_patterns.json @@ -37,5 +37,11 @@ "**/tests/**/*.elm", "**/vendor/**/*.{jsonnet,libsonnet}", "**/test/**/*.zig", - "**/*_test.zig" + "**/*_test.zig", + "**/kitex_gen/**/*.go", + "**/*.capnp.h", + "**/*.capnp.go", + "**/*.capnp.ts", + "**/*_capnp.rs", + "**/*_capnp.py" ] diff --git a/internal/config/allowlist/supported_file_types.json b/internal/config/allowlist/supported_file_types.json index 08741af84..ee7ebf778 100644 --- a/internal/config/allowlist/supported_file_types.json +++ b/internal/config/allowlist/supported_file_types.json @@ -91,5 +91,7 @@ ".pot", ".jsonnet", ".libsonnet", - ".zig" + ".zig", + ".thrift", + ".capnp" ] diff --git a/internal/config/rules/rule_docs/capnp.md b/internal/config/rules/rule_docs/capnp.md new file mode 100644 index 000000000..f32e7ebe7 --- /dev/null +++ b/internal/config/rules/rule_docs/capnp.md @@ -0,0 +1,36 @@ +> Favor precision over recall: only raise an issue when you are confident it is a real defect, and stay silent when the surrounding context is unclear — a false alarm costs more reviewer trust than a missed minor issue. Treat wire-compatibility breaks as blocking, and naming or layout preferences as non-blocking. + +#### Ordinals and Wire Compatibility +- Changing the `@N` ordinal of an existing field or method; the ordinal is that member's fixed slot, so it is the one thing that must never move +- Filling an ordinal left behind by a removed member instead of holding it with an `obsolete`/`obsoleteN` placeholder of the original width (`obsoleteSave @7 :AnyPointer`, `obsolete3 @3 :Bool`) +- Deleting a member outright rather than renaming it to `obsolete*` and leaving its ordinal and type in place +- Adding a member at an ordinal already used elsewhere in the same struct, union, or interface +- Do not report a rename that leaves the ordinal alone; names are not on the wire, so renaming is free +- Do not report declaration order that disagrees with ordinal order, which is legal and common (`rpc.capnp` declares `disembargo @13` above `obsoleteSave @7`) + +#### Types and Defaults +- Widening a fixed-width field, such as `UInt32` to `UInt64` or `Float32` to `Float64`: slots are fixed-width at fixed offsets, so this is a break, unlike widening a protobuf varint +- Any other change to an existing field's type, including a signedness flip or swapping an enum for the integer that backs it +- Changing the default value of an existing field; Cap'n Proto encodes values XOR the default, so the same bytes decode differently on either side of the change +- `Text` used to carry arbitrary bytes where `Data` is meant, since `Text` asserts NUL-terminated UTF-8 and readers may validate it +- Do not report a field appended at the next unused ordinal, which is backward compatible + +#### Unions, Groups, and Type IDs +- Moving an existing field into or out of a union or group, with one legal exception: wrapping an existing field in a brand-new union where it is the first member +- A union whose lowest ordinal is not a `Void` sentinel, leaving no representable "unset" state +- Adding a member to an existing union without confirming readers handle an unknown discriminant; older code sees a value outside the enum it was compiled against +- Renaming a struct, interface, or file with no explicit `@0x...` id pinned: the id is derived from the name, so the rename silently changes it and breaks anything holding the old one +- Do not report an explicit `@0x...` id carried through a rename; that is the fix, not the defect + +#### Interfaces and Methods +- Renumbering an existing method, or reusing the ordinal of one that was removed +- Changing an existing method's parameter or result struct in any way the field rules above forbid +- Removing a method rather than renaming it to `obsolete*` and keeping the ordinal (`sandstorm` keeps `obsoleteHttpGet @1` and `obsoleteGetGrainSize @3`) +- Capabilities returned with no documented lifetime, where dropping the client silently cancels work still in progress +- Do not report a method rename that keeps its ordinal + +#### Security and Resource Limits +- `AnyPointer` accepted from untrusted input and cast without a type check +- Unbounded `List`, `Text`, or `Data` from untrusted input with no traversal limit or nesting limit set on the reader +- Secrets, tokens, or credentials embedded in constants, defaults, or comments +- Do not report when reader limits are set at the call site and that boundary is clearly documented diff --git a/internal/config/rules/rule_docs/thrift.md b/internal/config/rules/rule_docs/thrift.md new file mode 100644 index 000000000..c144573c4 --- /dev/null +++ b/internal/config/rules/rule_docs/thrift.md @@ -0,0 +1,35 @@ +> Favor precision over recall: only raise an issue when you are confident it is a real defect, and stay silent when the surrounding context is unclear — a false alarm costs more reviewer trust than a missed minor issue. Treat wire-compatibility breaks as blocking, and naming or layout preferences as non-blocking. + +#### Field IDs and Wire Compatibility +- Reusing the id of a deleted field; Thrift has no `reserved` keyword, so a retired id must be held open by a placeholder field carrying a "do not reuse this id" comment +- Renumbering an existing field, or inserting a new field by shifting the ids of everything after it, instead of appending the next unused id +- Changing the declared type of an existing id, including `i32` to `i64` and swapping an enum for the integer that backs it; the type byte travels in the field header +- Deleting a field that peers still send without leaving its id held open for the same reason +- Do not report purely additive fields that take a fresh unused id, comment-only edits, or `namespace` and `include` changes + +#### Requiredness and Defaults +- Adding a `required` field to an existing struct: `required` is permanent and unskippable, so every existing peer fails to deserialize in both directions the moment one side adopts it +- Flipping an existing field between `required` and `optional`, which changes what a peer is allowed to omit +- Changing the default value of an existing optional field; an unset field and a field holding the default are indistinguishable to the peer, so the change lands silently +- Fields left with default requiredness where absence must be distinguishable from the zero value +- Do not report the choice of default requiredness itself when the file is internally consistent + +#### Services and Methods +- Renaming a service method: method names travel on the wire in `TMessageBegin`, unlike field names, so a rename breaks every existing caller +- Changing the ids of an existing method's parameters, or adding a parameter declared `required` +- Adding an exception to an existing `throws` clause that older clients have no branch to decode +- Changing a method to or from `oneway`, which changes whether the caller waits for a reply at all +- Do not report new methods appended to an existing service; those are backward compatible + +#### Enums and Constants +- Enum members declared without explicit numeric values, which makes every value positional and shifts them all on the first insertion +- Inserting a new enum member into the middle of an existing numeric range instead of appending +- Code that treats an unknown enum value as unreachable; peers on a newer schema will send values this build has never seen +- Do not report enum members appended with new explicit values + +#### Security and Resource Limits +- Unbounded `list`, `set`, `map`, `string`, or `binary` fields carried over an untrusted transport with no application-level size limit +- Recursive struct definitions with no documented depth bound on untrusted input +- Secrets, tokens, or credentials embedded in constants, default values, or comments +- `string` used to carry non-UTF-8 bytes where `binary` is meant, at a boundary that validates neither +- Do not report when limits are enforced by transport or server configuration and that boundary is clearly documented diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index 14af6d445..5aeabf13d 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -39,6 +39,8 @@ "**/*.swift": "swift.md", "**/*.elm": "elm.md", "**/*.{jsonnet,libsonnet}": "jsonnet.md", - "**/*.zig": "zig.md" + "**/*.zig": "zig.md", + "**/*.thrift": "thrift.md", + "**/*.capnp": "capnp.md" } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 8f5fdc25e..f75d157a9 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -129,6 +129,10 @@ func TestResolve_DefaultRules(t *testing.T) { {"analysis/plots.r", "R Code Review Principles"}, {"src/main.zig", "Illegal Behavior"}, {"build.zig", "Illegal Behavior"}, + {"idl/service.thrift", "Field IDs and Wire Compatibility"}, + {"if/common.thrift", "Field IDs and Wire Compatibility"}, + {"schema/addressbook.capnp", "Ordinals and Wire Compatibility"}, + {"src/rpc.capnp", "Ordinals and Wire Compatibility"}, } for _, tt := range tests { diff --git a/pages/src/content/docs/en/review-rules.md b/pages/src/content/docs/en/review-rules.md index f67d5dc20..f9ed1de89 100644 --- a/pages/src/content/docs/en/review-rules.md +++ b/pages/src/content/docs/en/review-rules.md @@ -180,6 +180,8 @@ matching order: | `**/*.bicep` | `bicep.md` — Bicep (Azure) templates. | | `**/*.elm` | `elm.md` - Elm source. | | `**/*.{jsonnet,libsonnet}` | `jsonnet.md` — Jsonnet configuration templates and libraries. | +| `**/*.thrift` | `thrift.md` — Apache Thrift IDL wire compatibility. | +| `**/*.capnp` | `capnp.md` — Cap'n Proto schema wire compatibility. | | *(fallback)* | `default.md` | The resolved rule body becomes the `{{system_rule}}` placeholder in the diff --git a/pages/src/content/docs/ja/review-rules.md b/pages/src/content/docs/ja/review-rules.md index c35ea98b5..92108867a 100644 --- a/pages/src/content/docs/ja/review-rules.md +++ b/pages/src/content/docs/ja/review-rules.md @@ -142,6 +142,8 @@ OCR は [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.bicep` | `bicep.md`: Bicep（Azure）テンプレート。 | | `**/*.elm` | `elm.md` - Elm ソースコード。 | | `**/*.{jsonnet,libsonnet}` | `jsonnet.md`: Jsonnet の設定テンプレートとライブラリ。 | +| `**/*.thrift` | `thrift.md`: Apache Thrift IDL のワイヤ互換性。 | +| `**/*.capnp` | `capnp.md`: Cap'n Proto スキーマのワイヤ互換性。 | | *(fallback)* | `default.md` | 解決されたルール本文は、plan および main task prompt 内の `{{system_rule}}` プレースホルダーの内容になります。 diff --git a/pages/src/content/docs/ru/review-rules.md b/pages/src/content/docs/ru/review-rules.md index 68c7b54b3..92fa3a58e 100644 --- a/pages/src/content/docs/ru/review-rules.md +++ b/pages/src/content/docs/ru/review-rules.md @@ -182,6 +182,8 @@ OCR использует [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com | `**/*.bicep` | `bicep.md` — шаблоны Bicep (Azure). | | `**/*.elm` | `elm.md` - исходный код Elm. | | `**/*.{jsonnet,libsonnet}` | `jsonnet.md` — шаблоны конфигурации и библиотеки Jsonnet. | +| `**/*.thrift` | `thrift.md` — совместимость Apache Thrift IDL на уровне wire. | +| `**/*.capnp` | `capnp.md` — совместимость схем Cap'n Proto на уровне wire. | | *(fallback)* | `default.md` | Разрешённое тело правила становится значением плейсхолдера `{{system_rule}}` diff --git a/pages/src/content/docs/zh/review-rules.md b/pages/src/content/docs/zh/review-rules.md index 5f6d94f83..e9be014b2 100644 --- a/pages/src/content/docs/zh/review-rules.md +++ b/pages/src/content/docs/zh/review-rules.md @@ -163,6 +163,8 @@ OCR 用 [`bmatcuk/doublestar/v4`](https://pkg.go.dev/github.com/bmatcuk/doublest | `**/*.bicep` | `bicep.md`——Bicep（Azure）模板。 | | `**/*.elm` | `elm.md` - Elm 源代码。 | | `**/*.{jsonnet,libsonnet}` | `jsonnet.md`——Jsonnet 配置模板与库。 | +| `**/*.thrift` | `thrift.md`——Apache Thrift IDL 线协议兼容性。 | +| `**/*.capnp` | `capnp.md`——Cap'n Proto schema 线协议兼容性。 | | *(fallback)* | `default.md` | 解析出的规则正文成为 plan 和 main task prompt 中 `{{system_rule}}` 占位符的内容。