This is an automated email from the ASF dual-hosted git repository.
chaokunyang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/fory.git
The following commit(s) were added to refs/heads/main by this push:
new 6da0beda9 docs: add fory code review skill (#3500)
6da0beda9 is described below
commit 6da0beda9f079cda445055b77dbe4cb11ae65861
Author: Shawn Yang <[email protected]>
AuthorDate: Mon Mar 23 16:26:03 2026 +0800
docs: add fory code review skill (#3500)
## Why?
## What does this PR do?
## Related issues
## AI Contribution Checklist
- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
## Does this PR introduce any user-facing change?
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
## Benchmark
---
.claude/skills/fory-code-review/SKILL.md | 71 ++++++++++++++++++++
.claude/skills/fory-code-review/agents/openai.yaml | 21 ++++++
.../references/lesson-derived-red-flags.md | 45 +++++++++++++
.../references/review-checklist.md | 53 +++++++++++++++
.../references/validation-command-matrix.md | 77 ++++++++++++++++++++++
5 files changed, 267 insertions(+)
diff --git a/.claude/skills/fory-code-review/SKILL.md
b/.claude/skills/fory-code-review/SKILL.md
new file mode 100644
index 000000000..e965e4f53
--- /dev/null
+++ b/.claude/skills/fory-code-review/SKILL.md
@@ -0,0 +1,71 @@
+---
+name: fory-code-review
+description: Review Apache Fory pull requests, branches, commits, or diffs
with a Fory-specific checklist. Use when auditing code in this repository for
protocol or xlang regressions, performance or benchmark-methodology issues,
cross-language inconsistencies, accidental public API growth, runtime ownership
drift, missing tests, or docs/spec mismatches. Also use before posting Fory
review findings inline to GitHub.
+---
+
+# Fory Code Review
+
+## Mission
+
+Find the highest-value bugs, regressions, and missing verification in Apache
Fory changes. Prioritize correctness, protocol safety, performance discipline,
and maintainability over style-only comments.
+
+## Start Here
+
+1. If reviewing against main, run `git fetch apache main` before diffing.
+2. Inspect the changed files first and cluster them by subsystem.
+3. Load only the references needed for the touched areas:
+ - `references/review-checklist.md`
+ - `references/lesson-derived-red-flags.md`
+ - `references/validation-command-matrix.md`
+
+## Review Workflow
+
+1. Define the review target.
+
+- Determine whether the user wants a review of a PR, branch, commit range, or
local diff.
+- Prefer `git diff --stat` first, then inspect the full patch only for touched
subsystems.
+
+2. Load focused context.
+
+- Protocol, type mapping, xlang, `TypeMeta`, `TypeInfo`, ref tracking, or
schema evolution changes:
+ - Read the relevant `docs/specification/**` sections before reviewing
behavior.
+- Benchmark or performance changes:
+ - Review both benchmark code and generated `docs/benchmarks/**` artifacts.
+- Runtime cleanup or cross-language alignment changes:
+ - Compare the changed ownership/API shape to the reference runtimes first,
usually C++ then Rust/Java.
+
+3. Inspect in this priority order.
+
+- Correctness, data corruption, security, and protocol drift.
+- Cross-language consistency and native/xlang behavior boundaries.
+- Performance regressions or invalid benchmark methodology.
+- Public API growth, legacy shims, wrapper layers, and architecture drift.
+- Missing tests, wrong test placement, and missing docs/spec updates.
+
+4. Validate each finding.
+
+- Tie the finding to exact changed lines.
+- Explain the concrete failure mode or regression risk.
+- State why the current code is wrong or incomplete, not only that it differs
from another style.
+- Recommend the missing test, benchmark, or spec/doc update when that is the
gap.
+
+5. Report findings.
+
+- Findings first, ordered by severity.
+- Keep overview and change summary brief and secondary.
+- If there are no findings, say that explicitly and mention residual risks or
testing gaps.
+
+## Hard Rules
+
+- Do not lead with style nits when there are correctness or verification risks.
+- Treat benchmark-shape tricks, payload-specific caches, and methodology
changes as real findings.
+- Treat undocumented public API additions, compatibility shims, and one-line
wrapper growth as findings when they increase maintenance surface without clear
need.
+- Treat protocol or performance claims without verification evidence as
incomplete.
+- When stream read loops are involved, remember that `(0, nil)` can be
transient; do not assume immediate failure is correct.
+- If the user wants comments posted on GitHub, produce findings suitable for
inline comments, then use `gh-pr-inline-review` to publish them.
+
+## Output Expectations
+
+- Use clickable file references with line numbers.
+- Keep each finding concrete: impact, evidence, and required fix.
+- Mention missing verification commands when the patch touches protocol,
performance, or cross-language behavior.
diff --git a/.claude/skills/fory-code-review/agents/openai.yaml
b/.claude/skills/fory-code-review/agents/openai.yaml
new file mode 100644
index 000000000..2b423c171
--- /dev/null
+++ b/.claude/skills/fory-code-review/agents/openai.yaml
@@ -0,0 +1,21 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+interface:
+ display_name: "Fory Code Review"
+ short_description: "Review Apache Fory changes for protocol, perf, and API
risks"
+ default_prompt: "Review Apache Fory changes with a Fory-specific checklist
focused on protocol correctness, cross-language consistency, benchmark rigor,
API discipline, missing tests, and docs/spec gaps."
diff --git
a/.claude/skills/fory-code-review/references/lesson-derived-red-flags.md
b/.claude/skills/fory-code-review/references/lesson-derived-red-flags.md
new file mode 100644
index 000000000..f27a0766f
--- /dev/null
+++ b/.claude/skills/fory-code-review/references/lesson-derived-red-flags.md
@@ -0,0 +1,45 @@
+# Lesson-Derived Red Flags
+
+## Protocol Drift
+
+- One runtime changed, but the same xlang behavior exists in other runtimes.
+- Helper names were unified, but signatures still hard-code mode-specific
types.
+- The patch adds explicit `if (xlang)` branching where the protocol is shared.
+- Compatible-mode or type-mapping semantics are described from memory instead
of current spec text.
+
+## Benchmark Methodology Problems
+
+- Benchmark-only config flags or shortcuts bypass real runtime work.
+- Serializer-specific model conversion happens inside timed loops without
being called out.
+- Before/after results were gathered from parallel benchmark runs on one host.
+- Reports or plots were not regenerated after benchmark logic changed.
+- Conclusions are drawn from smoke settings or noisy single-shot runs.
+
+## API And Architecture Drift
+
+- Removed APIs come back as aliases or shims.
+- Thin wrappers and pass-through helpers accumulate instead of being deleted.
+- Public API grows to fix an internal bug.
+- Language-local wrapper types, pending-state stacks, or side caches appear
without matching reference-runtime concepts.
+- Wrapper layers retain ownership that should live in resolver/context/buffer
types.
+
+## Streaming And Buffer Risks
+
+- Two objects appear to own one reader/writer index.
+- Stream bind or rebind paths do not detach stale backlinks.
+- Flush behavior depends on implicit side effects instead of one explicit
owner.
+- Review comments assume `(0, nil)` from a socket reader must be fatal
immediately.
+
+## Tests And Verification Gaps
+
+- Tests live in the wrong package or language suite for the behavior under
review.
+- Protocol or xlang changes lack the required cross-language test coverage.
+- Performance-sensitive changes have no benchmark or regression evidence.
+- Claimed cleanup is not backed by a full-tree search or exact command output.
+
+## Docs And Claim Precision
+
+- Language counts or support coverage are phrased as a fixed closed set.
+- Broad competitive claims are not split into concrete technical dimensions.
+- Titles or summaries overstate what the body proves.
+- Package-facing docs point to the wrong canonical destination.
diff --git a/.claude/skills/fory-code-review/references/review-checklist.md
b/.claude/skills/fory-code-review/references/review-checklist.md
new file mode 100644
index 000000000..47b840008
--- /dev/null
+++ b/.claude/skills/fory-code-review/references/review-checklist.md
@@ -0,0 +1,53 @@
+# Review Checklist
+
+## 1. Scope And Diff Setup
+
+- Identify the exact review target: `apache/main...HEAD`, PR branch, commit
range, or file subset.
+- Run `git diff --stat` before deep inspection.
+- If the review baseline is `apache/main`, refresh it first with `git fetch
apache main`.
+
+## 2. Protocol And Xlang
+
+- Did the patch touch `docs/specification/**`, `TypeMeta`, `TypeInfo`, type
IDs, ref flags, schema evolution, or xlang header/state?
+- Did the author update every affected runtime, or explicitly prove why the
other runtimes are already aligned?
+- Does the change preserve the intended boundary: xlang changed only where
required, native unchanged unless explicitly requested?
+- Are cross-language tests required but missing?
+- Does the implementation follow current spec text instead of a remembered
older behavior?
+
+## 3. Runtime Ownership And API Shape
+
+- Does the patch move ownership to the right layer (`TypeResolver`,
`ReadContext`, `WriteContext`, buffer/stream owner) instead of bloating `Fory`
facades?
+- Does it introduce wrappers, carrier objects, pending-state stacks, or side
caches that do not exist in the reference runtimes without clear evidence?
+- Does it reintroduce removed APIs, aliases, or compatibility shims against
explicit direction?
+- Are there new one-line forwarding helpers that should be inlined?
+- Are mode booleans or mode-specific parameter types leaking into hot shared
paths?
+
+## 4. Performance And Benchmarks
+
+- Does the patch optimize the runtime rather than the benchmark harness?
+- Does it depend on repeated payload identity, root-only shortcuts, or
fixture-specific behavior?
+- Were benchmark comparisons run sequentially, not concurrently?
+- If benchmark logic/reporting changed, were markdown reports and plots under
`docs/benchmarks/**` refreshed?
+- Are throughput and size comparisons apples-to-apples across languages and
serializer modes?
+
+## 5. Tests And Verification
+
+- Are tests in the right subsystem instead of whichever suite was convenient?
+- Are there targeted regressions tests for the changed behavior?
+- If protocol or xlang behavior changed, is the relevant xlang matrix called
out?
+- If performance-sensitive code changed, is there benchmark or regression
evidence?
+- Do the reported commands match the touched language/module?
+
+## 6. Docs And Public Claims
+
+- Are README, guides, specs, or benchmark docs required by the code change?
+- Do the docs use precise terminology and current semantics?
+- Are tables, captions, and summaries consistent with the body text?
+- Are canonical links and references present where the patch depends on prior
docs or published articles?
+
+## 7. Review Output
+
+- Findings first.
+- Order by severity.
+- Include file/line references.
+- If no findings, still state residual risk or testing gaps.
diff --git
a/.claude/skills/fory-code-review/references/validation-command-matrix.md
b/.claude/skills/fory-code-review/references/validation-command-matrix.md
new file mode 100644
index 000000000..8745de40a
--- /dev/null
+++ b/.claude/skills/fory-code-review/references/validation-command-matrix.md
@@ -0,0 +1,77 @@
+# Validation Command Matrix
+
+Use the smallest command set that proves the changed behavior. If protocol or
xlang behavior changed, call out the cross-language tests that should run even
if the author did not run them yet.
+
+## Repo-Wide Anchors
+
+- Format/lint sweep: `bash ci/format.sh --all`
+- Refresh remote main before main-branch comparison: `git fetch apache main`
+
+## Java
+
+- Build/test from `java/`
+- Typical checks:
+ - `mvn -T16 spotless:check`
+ - `mvn -T16 checkstyle:check`
+ - `mvn -T16 test`
+ - targeted: `mvn -T16 test -Dtest=<Class>#<method>`
+
+## C#
+
+- Build/test from `csharp/`
+- Typical checks:
+ - `dotnet format Fory.sln --verify-no-changes`
+ - `dotnet build Fory.sln -c Release --no-restore`
+ - `dotnet test Fory.sln -c Release`
+
+## C++
+
+- Build/test from repo root or `cpp/`
+- Architecture note: only add `--config=x86_64` on `x86_64` or `amd64`, not on
arm64.
+- Typical checks:
+ - `bazel build //cpp/...`
+ - `bazel test $(bazel query //cpp/...)`
+
+## Python
+
+- Work from `python/`
+- Typical checks:
+ - `ruff format .`
+ - `ruff check .`
+ - `ENABLE_FORY_CYTHON_SERIALIZATION=0 pytest -v -s .`
+ - `ENABLE_FORY_CYTHON_SERIALIZATION=1 pytest -v -s .`
+
+## Rust
+
+- Work from `rust/`
+- Typical checks:
+ - `cargo fmt --check`
+ - `cargo clippy --all-targets --all-features -- -D warnings`
+ - `cargo test --features tests`
+
+## Swift
+
+- Work from `swift/`
+- Typical checks:
+ - `swiftlint lint --config .swiftlint.yml`
+ - `swift build`
+ - `swift test`
+
+## Go
+
+- Work from `go/fory/`
+- Typical checks:
+ - `go fmt ./...`
+ - `go test -v ./...`
+
+## Xlang Matrix Triggers
+
+When the patch touches xlang behavior, type mapping, protocol bytes,
compatible mode, `TypeMeta`, or cross-language container semantics, require the
relevant Java-driven xlang tests:
+
+- `org.apache.fory.xlang.CPPXlangTest`
+- `org.apache.fory.xlang.CSharpXlangTest`
+- `org.apache.fory.xlang.RustXlangTest`
+- `org.apache.fory.xlang.GoXlangTest`
+- `org.apache.fory.xlang.PythonXlangTest`
+
+If the touched language is Swift and xlang behavior changed, include
`org.apache.fory.xlang.SwiftXlangTest` too.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
