Files
GenosseFlosse cfbb62e229 feat(allowlist): add matlab support (#574)
* 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>
2026-08-25 20:11:01 +08:00

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
}