mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-09-14 19:59:52 +08:00
cfbb62e229
* feat(rules): disambiguate .m files between MATLAB and Objective-C
.m is shared by MATLAB and Objective-C, so mapping **/*.m to matlab.md on
path alone gives Objective-C files MATLAB-specific review guidance. Add
matlab.md plus an objc.md placeholder, and decorate the system rule layer
with a sniffer that peeks a .m file's first non-blank line, selecting
objc.md when it looks like Objective-C (#import, @interface, ...).
The sniffer wraps the *system layer* rather than the composed resolver:
user layers (custom / project / global) must keep outranking it, including
when a user rule sets merge_system_rule. Wrapping the outermost resolver
would let the sniff discard a user's own .m rule.
Content is read at the ref under review via `git show <ref>:<path>`, so
the sniff is correct when that ref is not checked out; workspace reviews,
scan, and `ocr rules check` pass no ref and read the working tree. Any
read failure falls back to matlab.md, matching pre-sniff behavior.
The Resolver interface is unchanged. The sniffer forwards ResolveDetail
(so `ocr rules check` keeps working, annotating the pattern as
"(sniffed: objc)") and CanonicalConfig with the objc rule folded in, so
editing objc.md still invalidates the run manifest's rule_config_sha256.
objc.md ships as a copy of default.md: it is selected by the sniff rather
than from path_rule_map, so it is a neutral checklist to be filled in with
Objective-C specifics later.
Callers no longer lowercase paths before resolving: the resolver already
lowercases internally for glob matching, and passing a pre-lowered path
broke content reads for mixed-case paths.
* fix(rules): widen .m ObjC sniff and keep the result out of Pattern
Addresses review feedback on the .m disambiguation.
The sniff missed most real Objective-C files. peekFirstLine reads only
the first non-blank line, and that line is almost never the #import:
Xcode's file template opens with a "//" banner and most projects open
with a license header, so an ObjC file was getting the whole MATLAB
checklist — worse than the pre-feature fallback to default.md.
Reading N lines would only be a guess at how deep the directive sits, so
widen the signal set instead: MATLAB comments start with "%" and a .m
file cannot legally begin with "/" in MATLAB, which makes a C-style
comment opener a reliable ObjC signal on its own. Also add #pragma,
"signal isn't on line 1" case had no coverage at all, which is how this
got through.
Separately, the sniff no longer annotates RuleDetail.Pattern. That field
flows into delegateRuleGroupJSON's json:"pattern" in a payload carrying a
schema_version, where the contract is "the glob that matched" — and
"**/*.m (sniffed: objc)" is not a glob. Anything downstream comparing
patterns, deduping on them, or copying one into a .opencodereview/rule.json
would break on it, and grepping for the suffix would turn it into an API.
Record it in a new internal-only RuleDetail.SniffedAs instead, leave
delegateRuleGroupJSON untouched, and surface it in `ocr rules check` as a
separate Note: line, where the output is not a contract.
Also drop reviewContentRef, which was a hand-rolled copy of
tool.ParseReviewMode + ReviewMode.RefValue that had to stay in sync by
hand — review_cmd.go already computed the mode a few lines below the call
site. Its six-case table test goes with it. Fix two stale comments: the
specialCaseRuleDocs comment named ResolveWithContent and LoadDefault,
neither of which exists any more, and a comment in
TestResolve_FallbackToDefault referenced .swift matching swift.md, which
is upstream's change and not part of this diff.
* docs(rules): document .m content sniffing in zh, ja and ru
review-rules.md exists in four locales and all of them carry the
path_rule_map table; only the English one had the **/*.m row and the
content-sniffing section. Add both to zh, ja and ru, and update all four
for the widened signal set and the new Note: line in `ocr rules check`.
The ja and ru section headings carry an explicit {#content-sniffing-for-m-files}
id, and their "rule file format" headings an explicit
{#rule-file-format-layers-1-3}. generateHeadingId (pages/src/utils/headingId.ts)
only keeps [a-z0-9] plus CJK ideographs, so Cyrillic and Japanese kana
collapse away entirely: both new section headings would have slugged to a
bare "m" and the two cross-references would have pointed at nothing. The
explicit-id marker is the mechanism MarkdownRenderer already supports and
tests for exactly this case. zh needs none — its headings are ideographs,
which survive slugging.
* test(rules): use setTestHome in sniffer_test.go for Windows isolation
Upstream's setTestHome (test_home_test.go) sets both HOME and USERPROFILE
because os.UserHomeDir() prefers USERPROFILE on Windows, so a plain
t.Setenv("HOME", ...) still leaks into the developer's real
~/.opencodereview there. sniffer_test.go predates that helper; switch its
five call sites to match the rest of the package.
* fix(rules): trim matlab.md to defect-catching, general-audience rules
Addresses a line-by-line content review of matlab.md, which had not had
one before. This is the {{system_rule}} substitution in
prompts/main_task_user.md — the whole file lands in the model's "Review
Checklist" verbatim on every .m file, so anything in it that isn't a real
MATLAB defect is a steady source of false positives, which the doc's own
preamble ("favor precision over recall") is explicitly trying to avoid.
Removed as team-specific rather than general MATLAB defects:
- The German-only comment/identifier rule: an internal styleguide leaking
into a rule every user gets.
- "scripts are not permitted": MATLAB script files (no function keyword)
are a first-class, extremely common file type; banning them is a team
opinion, not a defect.
Fixed as factually wrong:
- `end` listed among shadowable built-ins alongside length/size/sum. end
is a keyword, not a function — `end = 5` is a syntax error in MATLAB,
so the model was being asked to find something that cannot occur.
- `snake_case` as the required casing: MathWorks' own code and the
common community style guides use lowerCamelCase. Corrected to match.
- `b_`/`l_` prefixes on logical variables: Hungarian notation, not a
MATLAB convention. Kept the `is` prefix, which is.
Reworded as diff-unscopable, conflicting with the preamble's "review only
the lines changed in this diff":
- Leading-function-name-matches-file-name and missing %% section markers
are both whole-file/whole-function structural properties that cannot be
judged from a changed hunk alone. Scoped both to new files / functions
the diff actually adds or substantially rewrites.
- Mandatory arguments (Input)/(Output) on every function flags all
existing code once a repo adopts the pattern anywhere. Scoped to new
functions in a file that already uses the pattern elsewhere, so it
stops flagging unrelated existing functions and repos that don't use
it at all.
- Dropped "the file should show a clean checkmark" from the Code
Analyzer rule; the rest of that line already scopes to changed lines
correctly.
Verified against the reviewer's own repro (a function using neither
arguments blocks nor snake_case) that none of the three previously-firing
rules apply to it under the new wording.
Left untouched per the review: everything else, including the technical
core (for k=v iterating columns, integer division rounding, ' vs .' on
complex data, inv(A)*b, struct("field", someCell) producing a struct
array).
* fix(cmd): update manual_e2e_retry_test.go for the .m content-ref param
The file sits behind //go:build manual_e2e, which go build/vet/test never
pass (nor does CI), so it stayed invisible to every check across the
earlier loadCommonContext signature change and only surfaced when the
reviewer ran `go vet -tags manual_e2e` directly. Workspace mode, so an
empty content ref, matching the other seven call sites.
* fix(rules): widen .m ObjC sniff to #if/#define, guard against Octave
#ifdef/#ifndef are collapsed into #if, which already prefix-matches both,
and now also catches a bare #if TARGET_OS_IPHONE platform guard. Added
#define for macro-first files. Deliberately not widened to a bare "#":
Octave, which also uses .m, treats # as a comment character, so that
would misclassify a real Octave/MATLAB file as Objective-C — covered by
a new test case. Docs updated in all four locales to match.
* fix(rules): scope the function-description-header rule to the diff
Same whole-file problem as the two rules already fixed in the prior
commit: whether a function has a description header isn't visible from a
changed hunk alone. Scoped to a function this diff adds or substantially
rewrites, matching the wording already used for the other two.
* fix(agent): drop ToLower before resolveGroupSystemRule's Resolve call
Rebasing onto upstream's new grouped-review path (executeSubtask was
removed entirely in favor of executeGroupSubtask) surfaced the same bug
already fixed at three other call sites: resolveGroupSystemRule lowercased
the path before calling Resolve. The resolver already lowercases
internally for glob matching, so this only broke the sniffer's file I/O —
a mixed-case .m path like ios/ViewController.m became
ios/viewcontroller.m, which doesn't exist on a case-sensitive filesystem
or in git, so the content peek silently failed and fell back to
matlab.md.
This is the only review dispatch path left after the rebase (single-file
executeSubtask is gone), so the bug would have hit every real run rather
than just a specific mode. Added a regression test building a real
sniffer-backed resolver against a temp git repo with a mixed-case ObjC
path; confirmed it fails without the fix and passes with it.
* docs(rules): trim .m sniffing docs to behavior-only, add stability caveat
- Reduce content-sniffing documentation from ~50 lines per language to ~10
lines: remove implementation details (prefix rationale, Octave reasoning,
ref-reading internals, objc.md placeholder discussion, ocr rules check
example output) that over-expose the heuristic as a stable contract.
- Keep only: what it does, fallback behavior, and a stability note warning
that the heuristic may change between versions.
- Add json:"-" tag to RuleDetail.SniffedAs to prevent accidental
serialization if the struct is ever marshaled to JSON.
---------
Co-authored-by: f.preuschoff <genosseflosse@users.noreply.github.com>
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
83 lines
2.3 KiB
Go
83 lines
2.3 KiB
Go
// SPDX-License-Identifier: Apache-2.0
|
|
// Copyright 2026 alibaba/open-code-review Contributors
|
|
|
|
package main
|
|
|
|
import (
|
|
"fmt"
|
|
"strings"
|
|
|
|
"github.com/alibaba/open-code-review/internal/config/rules"
|
|
"github.com/spf13/cobra"
|
|
)
|
|
|
|
var rulesCmd = &cobra.Command{
|
|
Use: "rules",
|
|
Short: "Inspect and debug review rules",
|
|
Long: "Inspect and debug review rules.",
|
|
Args: cobra.NoArgs,
|
|
RunE: func(cmd *cobra.Command, args []string) error {
|
|
return cmd.Help()
|
|
},
|
|
}
|
|
|
|
var rulesCheckRepoDir string
|
|
var rulesCheckRulePath string
|
|
|
|
var rulesCheckCmd = &cobra.Command{
|
|
Use: "check [flags] <file-path>",
|
|
Short: "Show which review rule applies to a given file path",
|
|
Long: "Show which review rule applies to the given file path, including its source layer and matched pattern.",
|
|
Example: ` ocr rules check src/main/java/com/example/Foo.java
|
|
ocr rules check --rule custom.json src/main/resources/mapper/UserMapper.xml`,
|
|
Args: exactArgs(1),
|
|
RunE: func(cmd *cobra.Command, args []string) error {
|
|
return runRulesCheck(args[0])
|
|
},
|
|
}
|
|
|
|
func init() {
|
|
addRepoFlag(rulesCheckCmd, &rulesCheckRepoDir)
|
|
rulesCheckCmd.Flags().StringVar(&rulesCheckRulePath, "rule", "", "path to a custom rule JSON file")
|
|
rulesCmd.AddCommand(rulesCheckCmd)
|
|
}
|
|
|
|
func runRulesCheck(filePath string) error {
|
|
resolvedRepo, err := resolveRepoDir(rulesCheckRepoDir)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
|
|
resolver, _, err := rules.NewResolver(resolvedRepo, rulesCheckRulePath, rules.ResolverOptions{})
|
|
if err != nil {
|
|
return fmt.Errorf("load rules: %w", err)
|
|
}
|
|
|
|
dr, ok := resolver.(rules.DetailResolver)
|
|
if !ok {
|
|
return fmt.Errorf("resolver does not support detail inspection")
|
|
}
|
|
|
|
detail := dr.ResolveDetail(filePath)
|
|
|
|
sourceLabel := map[string]string{
|
|
"custom": "Custom (--rule)",
|
|
"project": "Project (.opencodereview/rule.json)",
|
|
"global": "Global (~/.opencodereview/rule.json)",
|
|
"system": "System built-in",
|
|
}
|
|
|
|
fmt.Printf("File: %s\n", filePath)
|
|
fmt.Printf("Source: %s\n", sourceLabel[detail.Source])
|
|
fmt.Printf("Pattern: %s\n", detail.Pattern)
|
|
if detail.SniffedAs != "" {
|
|
fmt.Printf("Note: rule selected by file content (%s), not by path alone\n", detail.SniffedAs)
|
|
}
|
|
fmt.Println("Rule:")
|
|
fmt.Println(strings.Repeat("─", 40))
|
|
fmt.Println(detail.Rule)
|
|
fmt.Println(strings.Repeat("─", 40))
|
|
|
|
return nil
|
|
}
|