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}}` 占位符的内容。