From 671cc95985d0fa98658d505a21a415b5206263b7 Mon Sep 17 00:00:00 2001 From: "f.preuschoff" Date: Wed, 5 Aug 2026 16:08:19 +0200 Subject: [PATCH] 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 :`, 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. --- cmd/opencodereview/delegate_cmd.go | 2 +- cmd/opencodereview/review_cmd.go | 2 +- cmd/opencodereview/rules_cmd.go | 4 +- cmd/opencodereview/rules_cmd_test.go | 135 +++++++++ cmd/opencodereview/scan_cmd.go | 2 +- cmd/opencodereview/shared.go | 32 +- cmd/opencodereview/shared_test.go | 25 ++ internal/agent/agent.go | 2 +- internal/config/allowlist/allowed_ext_test.go | 1 + .../config/rules/canonical_config_test.go | 4 +- internal/config/rules/rule_docs/matlab.md | 145 +++++++++ internal/config/rules/rule_docs/objc.md | 22 ++ internal/config/rules/sniffer.go | 172 +++++++++++ internal/config/rules/sniffer_test.go | 284 ++++++++++++++++++ internal/config/rules/system_rules.go | 62 +++- internal/config/rules/system_rules.json | 3 +- internal/config/rules/system_rules_test.go | 67 +++-- internal/scan/agent.go | 2 +- pages/src/content/docs/en/review-rules.md | 34 +++ 19 files changed, 944 insertions(+), 56 deletions(-) create mode 100644 cmd/opencodereview/rules_cmd_test.go create mode 100644 internal/config/rules/rule_docs/matlab.md create mode 100644 internal/config/rules/rule_docs/objc.md create mode 100644 internal/config/rules/sniffer.go create mode 100644 internal/config/rules/sniffer_test.go diff --git a/cmd/opencodereview/delegate_cmd.go b/cmd/opencodereview/delegate_cmd.go index 2a2c15c00..23af0f0b3 100644 --- a/cmd/opencodereview/delegate_cmd.go +++ b/cmd/opencodereview/delegate_cmd.go @@ -89,7 +89,7 @@ type delegateContext struct { } func loadDelegateContext(opts delegateOptions) (*delegateContext, error) { - cc, err := loadCommonContext(opts.repoDir, opts.rulePath, 0, opts.maxGitProcs, true) + cc, err := loadCommonContext(opts.repoDir, opts.rulePath, reviewContentRef(opts.from, opts.to, opts.commit), 0, opts.maxGitProcs, true) if err != nil { return nil, err } diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index d4d394f66..e962f3cfd 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -102,7 +102,7 @@ func init() { } func executeReview(opts reviewOptions) error { - cc, err := loadCommonContext(opts.repoDir, opts.rulePath, opts.maxTools, opts.maxGitProcs, true) + cc, err := loadCommonContext(opts.repoDir, opts.rulePath, reviewContentRef(opts.from, opts.to, opts.commit), opts.maxTools, opts.maxGitProcs, true) if err != nil { return err } diff --git a/cmd/opencodereview/rules_cmd.go b/cmd/opencodereview/rules_cmd.go index 5c028c34b..ccf901f0a 100644 --- a/cmd/opencodereview/rules_cmd.go +++ b/cmd/opencodereview/rules_cmd.go @@ -48,7 +48,7 @@ func runRulesCheck(filePath string) error { return err } - resolver, _, err := rules.NewResolver(resolvedRepo, rulesCheckRulePath) + resolver, _, err := rules.NewResolver(resolvedRepo, rulesCheckRulePath, rules.ResolverOptions{}) if err != nil { return fmt.Errorf("load rules: %w", err) } @@ -58,7 +58,7 @@ func runRulesCheck(filePath string) error { return fmt.Errorf("resolver does not support detail inspection") } - detail := dr.ResolveDetail(strings.ToLower(filePath)) + detail := dr.ResolveDetail(filePath) sourceLabel := map[string]string{ "custom": "Custom (--rule)", diff --git a/cmd/opencodereview/rules_cmd_test.go b/cmd/opencodereview/rules_cmd_test.go new file mode 100644 index 000000000..47fbd424e --- /dev/null +++ b/cmd/opencodereview/rules_cmd_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func initRulesCheckTestRepo(t *testing.T) string { + t.Helper() + repo := t.TempDir() + git := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = repo + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + git("init") + git("config", "user.email", "t@t.co") + git("config", "user.name", "t") + return repo +} + +func writeRulesCheckTestFile(t *testing.T, repo, relPath, content string) { + t.Helper() + full := filepath.Join(repo, relPath) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// setRulesCheckRepo points the rulesCheckCmd's package-level --repo flag at +// repo for the duration of the test, restoring it afterward. runRulesCheck +// reads rulesCheckRepoDir directly (it's a singleton cobra command's bound +// flag var, not a per-call parameter), so tests must set it this way. +func setRulesCheckRepo(t *testing.T, repo string) { + t.Helper() + orig := rulesCheckRepoDir + rulesCheckRepoDir = repo + t.Cleanup(func() { rulesCheckRepoDir = orig }) +} + +// TestRunRulesCheck_ObjCSniffOverridesMatlab exercises peekFirstLine's actual +// disk-read path: system_rules.json maps "**/*.m" to matlab.md, but an +// Objective-C file (recognizable by its #import/@implementation header) +// should sniff away from that pattern and use the dedicated objc.md rule +// (rather than the incorrect MATLAB rule) instead. +func TestRunRulesCheck_ObjCSniffOverridesMatlab(t *testing.T) { + repo := initRulesCheckTestRepo(t) + writeRulesCheckTestFile(t, repo, "ios/ViewController.m", + "#import \"ViewController.h\"\n\n@implementation ViewController\n@end\n") + setRulesCheckRepo(t, repo) + + got := captureStdout(t, func() { + if err := runRulesCheck("ios/ViewController.m"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + if !strings.Contains(got, "Pattern: **/*.m (sniffed: objc)") { + t.Errorf("expected the sniffed-objc pattern label, got:\n%s", got) + } + if strings.Contains(got, "MATLAB") { + t.Errorf("expected MATLAB-specific guidance to be replaced by the objc rule, got:\n%s", got) + } +} + +// TestRunRulesCheck_MatlabFileStaysMatlab is the control case: a genuine +// MATLAB file (function header, no ObjC signals) must still resolve via the +// plain "**/*.m" pattern. +func TestRunRulesCheck_MatlabFileStaysMatlab(t *testing.T) { + repo := initRulesCheckTestRepo(t) + writeRulesCheckTestFile(t, repo, "Models/main.m", + "function y = main(x)\n y = x + 1;\nend\n") + setRulesCheckRepo(t, repo) + + got := captureStdout(t, func() { + if err := runRulesCheck("Models/main.m"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + if !strings.Contains(got, "Pattern: **/*.m") { + t.Errorf("expected the matlab pattern to still match, got:\n%s", got) + } +} + +// TestRunRulesCheck_MissingFileFallsBackToPathOnlyMatch covers peekFirstLine's +// error path: a file path that doesn't exist on disk (e.g. checking a rule +// before creating the file) must not error out — content sniffing is simply +// skipped and resolution falls back to plain path matching. +func TestRunRulesCheck_MissingFileFallsBackToPathOnlyMatch(t *testing.T) { + repo := initRulesCheckTestRepo(t) + setRulesCheckRepo(t, repo) + + got := captureStdout(t, func() { + if err := runRulesCheck("Models/does_not_exist.m"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + if !strings.Contains(got, "Pattern: **/*.m") { + t.Errorf("expected the matlab pattern to match by path alone, got:\n%s", got) + } +} + +// TestRunRulesCheck_BlankOnlyFileFallsBackToPathOnlyMatch covers +// peekFirstLine's other empty-result path: the file exists but has no +// non-blank line to sniff (e.g. only whitespace so far), so it must behave +// like no content was available rather than erroring. +func TestRunRulesCheck_BlankOnlyFileFallsBackToPathOnlyMatch(t *testing.T) { + repo := initRulesCheckTestRepo(t) + writeRulesCheckTestFile(t, repo, "Models/blank.m", "\n \n\t\n") + setRulesCheckRepo(t, repo) + + got := captureStdout(t, func() { + if err := runRulesCheck("Models/blank.m"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + if !strings.Contains(got, "Pattern: **/*.m") { + t.Errorf("expected the matlab pattern to match by path alone, got:\n%s", got) + } +} diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index 5d6a38879..883c701b7 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -105,7 +105,7 @@ func splitPaths(raw string) []string { } func executeScan(opts scanOptions) error { - cc, err := loadCommonContext(opts.repoDir, opts.rulePath, opts.maxTools, opts.maxGitProcs, false) + cc, err := loadCommonContext(opts.repoDir, opts.rulePath, "", opts.maxTools, opts.maxGitProcs, false) if err != nil { return err } diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index c0b5e7511..720a673e9 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -51,7 +51,11 @@ type commonContext struct { // requireGit=true fails fast when the directory is not a git repo (review // path: diff concept requires git). requireGit=false allows non-git // directories (scan path: provider falls back to filepath.Walk). -func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int, requireGit bool) (*commonContext, error) { +// +// contentRef is the git ref whose file content the rule resolver should +// inspect when disambiguating ambiguous extensions (see reviewContentRef). +// Pass "" to read the working tree, which is what scan wants. +func loadCommonContext(repoDirInput, rulePath, contentRef string, maxTools, maxGitProcs int, requireGit bool) (*commonContext, error) { tpl, err := template.LoadDefault() if err != nil { return nil, fmt.Errorf("load default template: %w", err) @@ -68,7 +72,14 @@ func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int, return nil, err } - resolver, fileFilter, err := rules.NewResolver(repoDir, rulePath) + // Built before the resolver: the sniffer reads file content at contentRef + // through this limiter. + gitRunner := gitcmd.New(maxGitProcs) + + resolver, fileFilter, err := rules.NewResolver(repoDir, rulePath, rules.ResolverOptions{ + Ref: contentRef, + Runner: gitRunner, + }) if err != nil { return nil, fmt.Errorf("load rules: %w", err) } @@ -78,11 +89,26 @@ func loadCommonContext(repoDirInput, rulePath string, maxTools, maxGitProcs int, RepoDir: repoDir, Resolver: resolver, FileFilter: fileFilter, - GitRunner: gitcmd.New(maxGitProcs), + GitRunner: gitRunner, IsGitRepo: isGit, }, nil } +// reviewContentRef returns the ref whose content the rule resolver should read, +// mirroring how diff.Provider picks the ref it passes to finalizeDiff: the head +// of the range in range mode, the commit in commit mode, and "" for workspace +// mode (where the working tree is the thing under review). +func reviewContentRef(from, to, commit string) string { + switch { + case commit != "": + return commit + case from != "" && to != "": + return to + default: + return "" + } +} + // resolveWorkingDir returns (absPath, isGitRepo, err). When requireGit is // true, returns an error if the directory is not a git repo. When false, // returns IsGitRepo=false instead of erroring (scan path uses this). diff --git a/cmd/opencodereview/shared_test.go b/cmd/opencodereview/shared_test.go index cce17868b..3a1652161 100644 --- a/cmd/opencodereview/shared_test.go +++ b/cmd/opencodereview/shared_test.go @@ -221,3 +221,28 @@ func TestResolveWorkingDir_GitRepo(t *testing.T) { } _ = isGit } + +// reviewContentRef decides which ref the rule resolver reads file content at +// when disambiguating ambiguous extensions. It must mirror how diff.Provider +// picks the ref it hands to finalizeDiff, or the sniff would inspect content +// from a different commit than the one under review. +func TestReviewContentRef(t *testing.T) { + tests := []struct { + name, from, to, commit, want string + }{ + {name: "commit mode wins", commit: "abc123", want: "abc123"}, + {name: "commit wins over a range", from: "main", to: "feat", commit: "abc123", want: "abc123"}, + {name: "range mode uses the head", from: "main", to: "feat", want: "feat"}, + {name: "workspace mode has no ref", want: ""}, + {name: "half a range is not a range", from: "main", want: ""}, + {name: "to without from is not a range", to: "feat", want: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := reviewContentRef(tt.from, tt.to, tt.commit); got != tt.want { + t.Errorf("reviewContentRef(%q, %q, %q) = %q, want %q", + tt.from, tt.to, tt.commit, got, tt.want) + } + }) + } +} diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 5764d250d..6eed5f911 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -1108,7 +1108,7 @@ func (a *Agent) executeSubtask(ctx context.Context, d model.Diff) (bool, *subtas // Build change-files list excluding current file changeFilesExcludingCurrent := a.buildChangeFilesExcept(newPath) - rule := a.resolveSystemRule(strings.ToLower(newPath)) + rule := a.resolveSystemRule(newPath) threshold := a.args.Template.PlanModeLineThreshold changeLines := d.Insertions + d.Deletions diff --git a/internal/config/allowlist/allowed_ext_test.go b/internal/config/allowlist/allowed_ext_test.go index aea733e0a..efb9f1860 100644 --- a/internal/config/allowlist/allowed_ext_test.go +++ b/internal/config/allowlist/allowed_ext_test.go @@ -45,6 +45,7 @@ func TestIsAllowedExt(t *testing.T) { {".JL", true}, {".hcl", true}, {".HCL", true}, + {".m", true}, {".tfvars", true}, {".TFVARS", true}, {".bicep", true}, diff --git a/internal/config/rules/canonical_config_test.go b/internal/config/rules/canonical_config_test.go index ecf611da3..6af0ce09b 100644 --- a/internal/config/rules/canonical_config_test.go +++ b/internal/config/rules/canonical_config_test.go @@ -48,7 +48,7 @@ func TestComposedResolverCanonicalConfig(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -88,7 +88,7 @@ func TestComposedResolverCanonicalConfig_ProjectRuleChangeChangesOutput(t *testi if err := os.WriteFile(filepath.Join(ocrDir, "rule.json"), []byte(ruleJSON), 0o644); err != nil { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } diff --git a/internal/config/rules/rule_docs/matlab.md b/internal/config/rules/rule_docs/matlab.md new file mode 100644 index 000000000..2b302a1ce --- /dev/null +++ b/internal/config/rules/rule_docs/matlab.md @@ -0,0 +1,145 @@ +> 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 correctness, data-integrity, and unsafe-dynamic-code findings as blocking; treat naming, comment, and idiom suggestions as non-blocking. Review only the lines changed in this diff. MATLAB resolves most names at run time — do not infer the behavior of functions, classes, or validators defined outside the file under review. + +#### Obvious Typos or Spelling Errors + +- Spelling errors in function names, local function names, variable names, struct field names, or `arguments` block parameter names at their declaration sites; do not report spelling errors at reference sites, as these are determined by the declaration +- Typos in `error`/`warning`/`assert` message text, error identifiers, `fprintf`/`disp` log output, or the function description header that affect readability +- Comments, identifiers, or log messages written in German rather than English + +#### File and Function Structure + +- A `.m` file that does not begin with the `function` keyword; scripts are not permitted +- A file whose leading function name does not match the file name +- Functions longer than roughly 200 lines that could be decomposed into local functions; report as non-blocking unless the length actively obscures a defect +- Nested functions used where a local function would do; nested functions share the parent workspace and should be reserved for cases that genuinely require shared access +- A helper called from exactly one parent function and placed in its own file instead of as a local function below the parent +- Missing `%%` section markers, or section markers without a description of what the section does, in a function long enough to need structure +- Do not report file length alone, and do not report structure findings on files that were only touched incidentally + +#### Argument Validation and Input Contracts + +- A function without an `arguments (Input)` block, or with an input block but no `arguments (Output)` block +- An `arguments` block placed after executable code rather than immediately following the function description header +- Parameters declared with neither a size, a class, nor a validator function — an empty declaration validates nothing +- Missing size specification where the shape is known (`(:,1)`, `(1,1)`, `(:,:)`); missing class specification where the type is known (`double`, `logical`, `string`, `struct`) +- Class validation coerces rather than rejects when a conversion exists: a parameter declared `double` silently converts `logical`, integer, and `char` inputs (`'a'` becomes `97`). Where the caller must not be silently converted, add `mustBeA` or an equivalent validator +- Optional arguments handled via `nargin` branching, `exist("var","var")`, or `isempty` checks where a default value in the input `arguments` block would express the same contract declaratively +- A default value assigned in an `arguments (Output)` block; output blocks do not support defaults +- Do not report a missing validator when the size and class declarations already constrain the input adequately + +#### Naming Conventions + +- `i` or `j` used as a loop counter or any other variable; both are built-in functions for the imaginary unit, and shadowing them silently changes complex arithmetic elsewhere in the function +- Any built-in shadowed by a variable name — `length`, `size`, `sum`, `max`, `min`, `end`, `error`, `table`, `str`, `time`, `power`, `line` are the common offenders. Treat as blocking when the shadowed built-in is called later in the same scope +- Function or variable names not in `snake_case`; single-letter or cryptic names where a descriptive name is possible; abbreviations that are not established domain terms +- Logical variables not prefixed with `is`, `b_`, or `l_` +- A variable reused within one function for a second purpose, or reassigned to a different class or array shape; this costs both readability and run time +- Do not report abbreviations that are standard in the domain and do not report established naming in surrounding untouched code + +#### Comments and Documentation + +- A function without a description header, or a header that restates the function name without saying what the function does, what it returns, and what the caller must guarantee +- Input or output variables described neither in the header nor as a trailing comment in the `arguments` blocks +- Non-obvious logic — index arithmetic, sign conventions, unit conversions, matrix assembly — left uncommented +- A comment that contradicts the code beside it; this is a correctness signal, not a style one, since one of the two is wrong +- Lines longer than 120 characters, or long expressions not broken across lines with `...` at logical boundaries +- Do not request comments on self-explanatory single-purpose lines, and do not report comment density in the abstract + +#### Dead Code and Diff Hygiene + +- Code that can never execute: statements after `return`, `error`, `break`, or `continue`; branches whose condition is a constant; `if false` blocks +- Variables assigned but never read, outputs computed but never returned, and input parameters never used — unless the signature is fixed by a callback or interface contract +- Large commented-out blocks with no note explaining why they are being preserved +- Whitespace-only or reindentation-only changes to lines the author did not otherwise modify; these create avoidable merge conflicts +- Legacy code rewritten purely to conform to the styleguide, with no functional change in the same hunk +- `%#ok<...>` suppressions of Code Analyzer warnings without an adjacent comment explaining why the warning is being ignored + +#### Indexing, Shapes, and Implicit Expansion + +- `for k = v` where `v` is a vector variable: the loop iterates over the *columns* of `v`, so a column vector yields exactly one iteration with the whole vector bound to `k`. Use `for k = 1:numel(v)` or transpose explicitly +- Reduction functions called without an explicit dimension (`sum(A)`, `max(A)`, `any(A)`, `mean(A)`) where `A` may be a single row at run time; MATLAB switches to row-wise behavior for row vectors. Pass the dimension: `sum(A,1)` +- Binary operations on operands with mismatched dimensions that silently broadcast under implicit expansion instead of erroring — for example `A + b` where `b` was intended to be conformable but is a row vector +- `&` or `|` inside an `if` condition with array operands, where `&&`/`||` with a scalar condition was intended; `if` requires *all* elements true, so this fails silently on mixed arrays +- `==` used to compare arrays of possibly different sizes in a condition; use `isequal` +- Indexing, `max`/`min`, or `x(1)`/`x(end)` on a container that can legitimately be empty upstream +- Repeated `find` calls where logical indexing would express the same filter, especially where several filters are combined +- Do not report shape assumptions that an `arguments` block size specification has already guaranteed + +#### Numeric Correctness + +- Floating-point values compared with `==` or `~=`, particularly convergence checks and tolerance comparisons; use an explicit tolerance or `ismembertol` +- `NaN` handling assumed rather than checked: `NaN == NaN` is false, `sum` propagates `NaN` while `max`/`min` skip it by default, and `isnan` is the only reliable test +- Integer-class arithmetic treated as C-like: MATLAB integer division *rounds to nearest* (`int32(5)/int32(2)` is `3`) and overflow *saturates* at `intmax` rather than wrapping. Use `idivide` with an explicit rounding mode where truncation is intended +- `single` and `double` mixed in one expression; the result silently degrades to `single` +- `'` (complex-conjugate transpose) used where `.'` (plain transpose) was intended on complex data such as phasors, impedance matrices, or admittance matrices — a defect that is invisible on real-valued test data +- Matrix operators used where element-wise was intended, or the reverse: `*` vs `.*`, `/` vs `./`, `^` vs `.^` +- `inv(A)*b` instead of `A\b`; the explicit inverse is slower and less accurate. `inv()` applied to a sparse matrix additionally destroys sparsity and can exhaust memory on network-sized systems +- Division by a quantity that can legitimately be zero (an out-of-service branch, a zero base value, an empty aggregate) without a guard +- Do not flag numerical style where the surrounding code documents a deliberate choice + +#### Error Handling, Assertions, and Logging + +- A condition that will inevitably lead to a downstream failure left unchecked; assert explicitly at the point where the assumption is made +- `assert` called without an error identifier, or with an identifier that does not follow `function_name:ErrorCondition` +- `try` blocks with an empty `catch`, a `catch` that only rethrows without context, or a `catch` that omits `disp(getReport(ME))` +- `try` blocks wrapping substantially more code than the one call that can actually fail, obscuring where the error originates +- A `catch` that swallows an error and continues with a partially computed result, so the caller sees plausible but wrong output +- Log output that reports nothing actionable, or that omits the project's expected context (timestamp, function name); a long-running function that produces no summary output at all on completion +- Do not report missing logging in small pure helper functions + +#### State, Scope, and Lifetime + +- Any use of `global`; the only tolerable exception is a logical feature flag such as a debug level, and even then it should be questioned in review +- `persistent` variables without a documented reset path; a stale cache surviving into the next calculation is a silent-wrong-answer defect +- `clear all`, `clear classes`, `close all`, or `clc` inside a function +- `warning("off", ...)` set without restoring the previous state, leaving warnings suppressed for the rest of the session; capture and restore the state. +- `assignin`, `evalin`, or `inputname` reaching into a caller's workspace +- Runtime `cd`, `addpath`, or `rmpath` +- Runtime introspection in a hot path: `exist`, `which`, `whos`, `dbstack` + +#### Data Types and Containers + +- `char` used for text where `string` would work; `char` breaks on ragged concatenation (`['asdf';'asd']` errors) and lacks `+` concatenation +- `cell` arrays used for homogeneous or tabular data where a `table`, a numeric matrix, or a struct array fits; each cell carries roughly 120 bytes of overhead +- `cell2mat` where `vertcat(c{:,1})` or `horzcat` would do the same job far more cheaply +- `struct("field", someCell)` — a cell value argument creates a *struct array*, not a struct holding a cell +- Dynamic field names built from data (`s.(name)`) where a `table`, `dictionary`, or `containers.Map` would express the lookup, and where a malformed name would error at run time +- `unique`, `sort`, or `setdiff` applied where the original row order matters, without `"stable"` +- Do not report container choice in code that is demonstrably not on a hot path and is already clear + +#### Performance and Preallocation + +Confirm the code is on a hot path and that the data scale justifies the finding before flagging: + +- Arrays grown inside a loop (`x(end+1) = ...`, `x = [x; new]`, `s(end+1).f = ...`) where the final size is known or boundable; preallocate instead +- Preallocation that no longer matches the final size after subsequent edits — an oversized preallocation leaves trailing zeros that silently enter the result +- Loop-invariant work inside a loop: repeated `ismember` against the same set, repeated struct field lookups, repeated table indexing, repeated file access +- Small element-wise loops that vectorize cleanly +- A variable that changes class or shape mid-function instead of a new variable being introduced +- `parfor` used before the serial version has been profiled; `parfor` without a `numThreads` argument, which makes debugging inside called functions impossible +- Loop-carried dependencies, order-dependent output, or shared mutable state inside `parfor`; results that depend on iteration order are a correctness defect, not a performance note +- Random number generation inside `parfor` without an explicit reproducible stream, where results must be repeatable +- Do not raise micro-optimizations; clear code that is slower is explicitly preferred to fast code that is hard to follow + +#### File and Data I/O + +- `load` or `save` without an explicit variable list; an unrestricted `load` can silently overwrite existing workspace variables +- `load` called without capturing the output struct inside a function +- `exist("name","file")` or `exist("name","dir")` instead of `isfile` / `isfolder` +- `xlsread` or `xlswrite` in new code; use `readtable`/`writetable`, `readmatrix`/`writematrix`, or `readcell`/`writecell` +- `fopen` without a guaranteed `fclose` on every exit path, including the error path; prefer `onCleanup` +- Paths assembled by string concatenation with hard-coded separators instead of `fullfile`; hard-coded absolute paths or drive letters + +#### Unsafe Dynamic Code + +- `eval`, `evalc`, or `feval` on a string assembled from data, file contents, or user input; this is arbitrary code execution +- `str2num` on any externally sourced value — it evaluates its argument; use `str2double` +- `system`, `dos`, or `unix` invoked with a command string built from unvalidated input +- File paths taken from external data and used without validation, allowing traversal outside the intended directory +- Credentials, tokens, or connection strings hard-coded in source or written to the log + +#### Compatibility and Code Analyzer + +- Toolbox-dependent functions used in code that is expected to run without that toolbox license +- Remaining Code Analyzer warnings in the changed lines; the file should show a clean checkmark +- Do not report a compatibility concern without naming the introducing release — an unverified claim here is worse than silence diff --git a/internal/config/rules/rule_docs/objc.md b/internal/config/rules/rule_docs/objc.md new file mode 100644 index 000000000..3a497dbbf --- /dev/null +++ b/internal/config/rules/rule_docs/objc.md @@ -0,0 +1,22 @@ +#### Correctness +Is the logic correct? Are there missing boundary conditions? +Are exceptions handled properly? +Is it thread-safe in concurrent scenarios? + +#### Security +Are there security vulnerabilities such as SQL injection or XSS? +Is sensitive information handled correctly? +Is permission validation complete? + +#### Performance +Are there obvious performance issues (e.g., N+1 queries, unnecessary loops)? +Are resources properly released? + +#### Maintainability +Is the code clear and easy to understand? +Do names accurately express intent? +Does it follow the project’s existing code style and architecture patterns? + +#### Test Coverage +Do critical logic paths have corresponding test cases? +Do test cases cover boundary conditions? diff --git a/internal/config/rules/sniffer.go b/internal/config/rules/sniffer.go new file mode 100644 index 000000000..800a1f425 --- /dev/null +++ b/internal/config/rules/sniffer.go @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package rules + +import ( + "bufio" + "bytes" + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/alibaba/open-code-review/internal/gitcmd" +) + +// sniffTimeout bounds a single content peek. A peek is a best-effort hint, so +// exceeding it yields "" (no sniff) rather than failing the whole resolution. +const sniffTimeout = 5 * time.Second + +// systemLayer is the subset of *SystemRule that composedResolver depends on. +// Declaring it lets the lowest layer be decorated (see sniffer) without any +// caller — or the Resolver interface itself — changing. +type systemLayer interface { + Resolve(path string) string + resolveDetail(path string) RuleDetail + CanonicalConfig() []string +} + +// sniffer decorates the system rule layer to disambiguate the ".m" extension, +// which MATLAB and Objective-C both use. system_rules.json maps "**/*.m" to +// matlab.md; when a ".m" file's first non-blank line looks like Objective-C, +// this returns objc.md instead. +// +// It wraps the *system layer* rather than the composed resolver on purpose: +// user-configured layers (custom / project / global) must keep outranking the +// system layer, including when a user rule sets merge_system_rule. Decorating +// the outermost resolver would let the sniff discard a user's own ".m" rule. +// +// It is deliberately stateless (no peek cache): Resolve is called from the +// concurrent per-file review goroutines, so caching would need a mutex, and a +// peek costs at most one read per ".m" file. +type sniffer struct { + inner systemLayer + repoDir string + ref string // git ref to read at; "" reads the working tree + runner *gitcmd.Runner + objcRule string // rule_docs/objc.md +} + +// Resolve returns objc.md for a ".m" file whose content sniffs as +// Objective-C, and otherwise defers to the wrapped system layer. +func (s *sniffer) Resolve(path string) string { + if s.sniffsAsObjC(path) { + return s.objcRule + } + return s.inner.Resolve(path) +} + +// resolveDetail mirrors Resolve while preserving the wrapped layer's matched +// pattern, annotating it so `ocr rules check` shows why matlab.md was +// overridden rather than silently reporting a different rule. +func (s *sniffer) resolveDetail(path string) RuleDetail { + detail := s.inner.resolveDetail(path) + if s.sniffsAsObjC(path) { + detail.Rule = s.objcRule + detail.Pattern += " (sniffed: objc)" + } + return detail +} + +// CanonicalConfig forwards the wrapped layer's fields and appends the objc +// rule text, so editing objc.md changes the run manifest's +// rule_config_sha256 exactly as editing any other rule doc does. +func (s *sniffer) CanonicalConfig() []string { + fields := s.inner.CanonicalConfig() + if s.objcRule != "" { + fields = append(fields, "layer", "system", "objc", s.objcRule) + } + return fields +} + +// sniffsAsObjC reports whether path is a ".m" file whose content identifies it +// as Objective-C. Non-".m" paths never trigger a read. +func (s *sniffer) sniffsAsObjC(path string) bool { + if !strings.HasSuffix(strings.ToLower(path), ".m") { + return false + } + return looksLikeObjC(s.peekFirstLine(path)) +} + +// peekFirstLine returns the first non-blank line of path, read at s.ref when +// set (so refs that are not checked out still resolve correctly) and from the +// working tree otherwise. Any failure yields "", which leaves the path-based +// match — matlab.md — in place. +func (s *sniffer) peekFirstLine(path string) string { + if s.ref != "" { + return firstNonBlankLine(s.showAtRef(path)) + } + + f, err := os.Open(filepath.Join(s.repoDir, path)) + if err != nil { + return "" + } + defer f.Close() + + scanner := bufio.NewScanner(f) + for scanner.Scan() { + if line := strings.TrimSpace(scanner.Text()); line != "" { + return line + } + } + return "" +} + +// showAtRef reads path's content at s.ref via `git show :`. +// Returns "" when the file does not exist at that ref or git fails. +func (s *sniffer) showAtRef(path string) string { + ctx, cancel := context.WithTimeout(context.Background(), sniffTimeout) + defer cancel() + + args := []string{"-c", "core.quotepath=false", "show", "--end-of-options", s.ref + ":" + path} + + if s.runner != nil { + out, err := s.runner.Output(ctx, s.repoDir, args...) + if err != nil { + return "" + } + return string(out) + } + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = s.repoDir + var stdout bytes.Buffer + cmd.Stdout = &stdout + if err := cmd.Run(); err != nil { + return "" + } + return stdout.String() +} + +// objcSniffPrefixes are first-line signals unique to Objective-C (import / +// interface declarations); MATLAB files begin with "function"/"classdef"/"%" +// and never produce any of these, so a false positive requires a MATLAB file +// to open with an actual Objective-C directive. +var objcSniffPrefixes = []string{"#import", "#include", "@interface", "@implementation", "@class", "@protocol"} + +// looksLikeObjC reports whether the first non-blank line of content looks +// like Objective-C rather than MATLAB. It returns false (keeping the default +// MATLAB behavior) when content is empty or inconclusive. +func looksLikeObjC(firstLine string) bool { + if firstLine == "" { + return false + } + for _, prefix := range objcSniffPrefixes { + if strings.HasPrefix(firstLine, prefix) { + return true + } + } + return false +} + +func firstNonBlankLine(content string) string { + for _, line := range strings.Split(content, "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + return trimmed + } + } + return "" +} diff --git a/internal/config/rules/sniffer_test.go b/internal/config/rules/sniffer_test.go new file mode 100644 index 000000000..f948c13f3 --- /dev/null +++ b/internal/config/rules/sniffer_test.go @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package rules + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/gitcmd" +) + +func writeFile(t *testing.T, dir, rel, content string) { + t.Helper() + full := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// initRepo creates a git repo with one commit containing the given files and +// returns the repo dir and the commit SHA. +func initRepo(t *testing.T, files map[string]string) (string, string) { + t.Helper() + dir := t.TempDir() + git := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) + } + git("init") + git("config", "user.email", "t@t.co") + git("config", "user.name", "t") + for rel, content := range files { + writeFile(t, dir, rel, content) + } + git("add", "-A") + git("commit", "-m", "init") + return dir, git("rev-parse", "HEAD") +} + +const ( + objcSource = "#import \"ViewController.h\"\n\n@implementation ViewController\n@end\n" + matlabSource = "function y = main(x)\n y = x + 1;\nend\n" +) + +// objcRuleText is the exact text a sniffed .m file must resolve to. Assert on +// identity rather than on wording: objc.md currently ships as a placeholder +// copy of default.md, so any substring check would be testing the placeholder. +func objcRuleText(t *testing.T) string { + t.Helper() + r, err := loadObjCRule() + if err != nil { + t.Fatal(err) + } + return r +} + +// resolveIn builds a real resolver for dir/ref and resolves path. +func resolveIn(t *testing.T, dir, ref, path string, runner *gitcmd.Runner) string { + t.Helper() + t.Setenv("HOME", t.TempDir()) // isolate from a real ~/.opencodereview/rule.json + r, _, err := NewResolver(dir, "", ResolverOptions{Ref: ref, Runner: runner}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + return r.Resolve(path) +} + +func TestSniffer_WorkingTree(t *testing.T) { + dir, _ := initRepo(t, map[string]string{ + "ios/ViewController.m": objcSource, + "Models/main.m": matlabSource, + "Models/blank.m": "\n \n\t\n", + "src/model.jl": objcSource, // ObjC-looking content, non-.m path + }) + + tests := []struct { + name, path string + wantObjC bool + wantSubstr string // checked when wantObjC is false + }{ + {name: "objc header resolves to objc rule", path: "ios/ViewController.m", wantObjC: true}, + {name: "matlab function header stays matlab", path: "Models/main.m", wantSubstr: "MATLAB"}, + {name: "blank-only file falls back to matlab", path: "Models/blank.m", wantSubstr: "MATLAB"}, + {name: "missing file falls back to matlab", path: "Models/absent.m", wantSubstr: "MATLAB"}, + {name: "non-.m path never sniffs", path: "src/model.jl", wantSubstr: "Type Stability"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := resolveIn(t, dir, "", tt.path, nil) + if tt.wantObjC { + if got != objcRuleText(t) { + t.Errorf("Resolve(%q): want the objc rule, got %q", tt.path, truncate(got, 80)) + } + return + } + if !strings.Contains(got, tt.wantSubstr) { + t.Errorf("Resolve(%q): want rule containing %q, got %q", tt.path, tt.wantSubstr, truncate(got, 80)) + } + }) + } +} + +// TestSniffer_ReadsAtRefNotCheckedOut is the reason the sniffer reads through +// `git show :` rather than the working tree: `ocr review --from/--to` +// can target a ref that is not checked out, and the file may not exist on disk +// at all. Reading the working tree would silently fall back to matlab.md. +func TestSniffer_ReadsAtRefNotCheckedOut(t *testing.T) { + dir, sha := initRepo(t, map[string]string{"ios/ViewController.m": objcSource}) + + // Remove it from the working tree; it now exists only in the commit. + if err := os.Remove(filepath.Join(dir, "ios/ViewController.m")); err != nil { + t.Fatal(err) + } + + t.Run("with ref: still sniffs as objc", func(t *testing.T) { + got := resolveIn(t, dir, sha, "ios/ViewController.m", nil) + if got != objcRuleText(t) { + t.Errorf("expected objc rule read at ref %s, got %q", sha, truncate(got, 80)) + } + }) + + t.Run("with ref via gitcmd.Runner", func(t *testing.T) { + got := resolveIn(t, dir, sha, "ios/ViewController.m", gitcmd.New(2)) + if got != objcRuleText(t) { + t.Errorf("expected objc rule via runner, got %q", truncate(got, 80)) + } + }) + + t.Run("without ref: file is gone, falls back to matlab", func(t *testing.T) { + got := resolveIn(t, dir, "", "ios/ViewController.m", nil) + if !strings.Contains(got, "MATLAB") { + t.Errorf("expected matlab fallback with no ref, got %q", truncate(got, 80)) + } + }) + + t.Run("unknown ref degrades to matlab rather than erroring", func(t *testing.T) { + got := resolveIn(t, dir, "0000000000000000000000000000000000000000", "ios/ViewController.m", nil) + if !strings.Contains(got, "MATLAB") { + t.Errorf("expected matlab fallback for a bad ref, got %q", truncate(got, 80)) + } + }) +} + +// TestSniffer_UserRuleOutranksSniff guards the layering: the sniffer decorates +// the *system* layer, so a user's own rule for a .m path must still win. If the +// sniffer were wrapped around the whole composed resolver it would short-circuit +// and discard the user rule. +func TestSniffer_UserRuleOutranksSniff(t *testing.T) { + dir, _ := initRepo(t, map[string]string{"ios/ViewController.m": objcSource}) + t.Setenv("HOME", t.TempDir()) + + writeFile(t, dir, ".opencodereview/rule.json", + `{"rules":[{"path":"**/*.m","rule":"MY PROJECT RULE"}]}`) + + r, _, err := NewResolver(dir, "", ResolverOptions{}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + if got := r.Resolve("ios/ViewController.m"); got != "MY PROJECT RULE" { + t.Errorf("user rule must outrank the objc sniff, got %q", truncate(got, 80)) + } +} + +// A user rule with merge_system_rule must receive the sniffed ObjC rule as its +// system half, not the MATLAB rule the path alone would select. +func TestSniffer_MergeSystemRuleUsesSniffedRule(t *testing.T) { + dir, _ := initRepo(t, map[string]string{"ios/ViewController.m": objcSource}) + t.Setenv("HOME", t.TempDir()) + + writeFile(t, dir, ".opencodereview/rule.json", + `{"rules":[{"path":"**/*.m","rule":"MY PROJECT RULE","merge_system_rule":true}]}`) + + r, _, err := NewResolver(dir, "", ResolverOptions{}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + got := r.Resolve("ios/ViewController.m") + if !strings.Contains(got, "MY PROJECT RULE") { + t.Error("merged rule lost the user half") + } + if !strings.Contains(got, objcRuleText(t)) { + t.Errorf("merged rule should carry the sniffed objc rule, got %q", truncate(got, 200)) + } + if strings.Contains(got, "Indexing, Shapes, and Implicit Expansion") { + t.Error("merged rule carried the MATLAB rule instead of the sniffed objc rule") + } +} + +func TestSniffer_ResolveDetailAnnotatesPattern(t *testing.T) { + dir, _ := initRepo(t, map[string]string{ + "ios/ViewController.m": objcSource, + "Models/main.m": matlabSource, + }) + t.Setenv("HOME", t.TempDir()) + + r, _, err := NewResolver(dir, "", ResolverOptions{}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + dr, ok := r.(DetailResolver) + if !ok { + t.Fatal("sniffer-wrapped resolver must still satisfy DetailResolver") + } + + sniffed := dr.ResolveDetail("ios/ViewController.m") + if sniffed.Pattern != "**/*.m (sniffed: objc)" { + t.Errorf("Pattern = %q, want %q", sniffed.Pattern, "**/*.m (sniffed: objc)") + } + if sniffed.Source != "system" { + t.Errorf("Source = %q, want system", sniffed.Source) + } + + plain := dr.ResolveDetail("Models/main.m") + if plain.Pattern != "**/*.m" { + t.Errorf("unsniffed Pattern = %q, want %q", plain.Pattern, "**/*.m") + } +} + +// The objc rule doc is not referenced from system_rules.json, so it must be +// folded into CanonicalConfig explicitly or editing it would not invalidate the +// run manifest's rule_config_sha256. +func TestSniffer_CanonicalConfigIncludesObjCRule(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + r, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + cc, ok := r.(interface{ CanonicalConfig() []string }) + if !ok { + t.Fatal("sniffer-wrapped resolver must still expose CanonicalConfig") + } + objcRule, err := loadObjCRule() + if err != nil { + t.Fatal(err) + } + fields := cc.CanonicalConfig() + found := false + for i := 0; i+3 < len(fields); i++ { + if fields[i+2] == "objc" && fields[i+3] == objcRule { + found = true + break + } + } + if !found { + t.Error("CanonicalConfig must include the objc rule text") + } +} + +func TestLooksLikeObjC(t *testing.T) { + objc := []string{"#import \"a.h\"", "#include ", "@interface Foo", "@implementation Foo", "@class Foo;", "@protocol Foo"} + for _, line := range objc { + if !looksLikeObjC(line) { + t.Errorf("looksLikeObjC(%q) = false, want true", line) + } + } + notObjc := []string{"", "function y = f(x)", "classdef Foo", "% a comment", "x = 1;"} + for _, line := range notObjc { + if looksLikeObjC(line) { + t.Errorf("looksLikeObjC(%q) = true, want false", line) + } + } +} + +func TestFirstNonBlankLine(t *testing.T) { + if got := firstNonBlankLine("\n\n #import \nrest\n"); got != "#import " { + t.Errorf("got %q", got) + } + if got := firstNonBlankLine("\n \t\n"); got != "" { + t.Errorf("blank-only should yield empty, got %q", got) + } +} diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index bafa23ec1..7752e3f7c 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -13,6 +13,8 @@ import ( "strings" "github.com/bmatcuk/doublestar/v4" + + "github.com/alibaba/open-code-review/internal/gitcmd" ) // Resolver resolves a review rule for a file path. @@ -111,6 +113,17 @@ func LoadDefault() (*SystemRule, error) { return &rule, nil } +// loadObjCRule reads the embedded Objective-C rule doc used by the ".m" +// content sniff. It is not referenced from system_rules.json's path_rule_map, +// so it is loaded explicitly rather than through the PathRules loop. +func loadObjCRule() (string, error) { + content, err := rulesFS.ReadFile("rule_docs/objc.md") + if err != nil { + return "", fmt.Errorf("read objc rule file: %w", err) + } + return strings.TrimRight(string(content), "\n"), nil +} + // RuleDetail contains the resolved rule along with metadata about its source. type RuleDetail struct { Rule string // rule text @@ -128,16 +141,7 @@ type DetailResolver interface { // The first match wins; if none match, it falls back to DefaultRule. // Supports full glob syntax including ** for recursive directory matching. func (r *SystemRule) Resolve(path string) string { - lowerPath := strings.ToLower(path) - for _, pr := range r.PathRules { - expanded := expandBraces(pr.Pattern) - for _, p := range expanded { - if matched, _ := doublestar.Match(strings.ToLower(p), lowerPath); matched { - return pr.Rule - } - } - } - return r.DefaultRule + return r.resolveDetail(path).Rule } // CanonicalConfig returns a deterministic, order-stable field list describing this @@ -255,7 +259,22 @@ type composedResolver struct { custom *ProjectRule // highest: --rule flag project *ProjectRule // high: .opencodereview/rule.json global *ProjectRule // low: ~/.opencodereview/rule.json - system *SystemRule // lowest: embedded default + system systemLayer // lowest: embedded default, decorated by sniffer +} + +// ResolverOptions carries the optional git context the resolver needs to read +// file content when disambiguating extensions shared by several languages +// (currently only ".m": MATLAB vs Objective-C). The zero value is valid and +// makes content reads fall back to the working tree. +type ResolverOptions struct { + // Ref is the git ref whose content should be inspected — the review head + // (--to) in range mode, or --commit in commit mode. Empty reads the + // working tree, which is what `ocr scan` and `ocr rules check` want. + Ref string + + // Runner bounds concurrent git subprocesses. Optional; when nil the + // resolver shells out to git directly. + Runner *gitcmd.Runner } // NewResolver builds a Resolver with the following priority: @@ -264,13 +283,22 @@ type composedResolver struct { // 3. Global ~/.opencodereview/rule.json (first match wins) // 4. Embedded system default rules // +// The system layer is wrapped in a sniffer so ".m" files can be resolved as +// Objective-C when their content says so. Wrapping the *system* layer (rather +// than the composed resolver) keeps user layers outranking the sniff. +// // It also returns a FileFilter with the merged include/exclude patterns from all layers. -func NewResolver(repoDir, customRulePath string) (Resolver, *FileFilter, error) { +func NewResolver(repoDir, customRulePath string, opts ResolverOptions) (Resolver, *FileFilter, error) { sysRule, err := LoadDefault() if err != nil { return nil, nil, err } + objcRule, err := loadObjCRule() + if err != nil { + return nil, nil, err + } + var customRule *ProjectRule if customRulePath != "" { cr, err := loadRuleFile(customRulePath) @@ -300,7 +328,13 @@ func NewResolver(repoDir, customRulePath string) (Resolver, *FileFilter, error) custom: customRule, project: projectRule, global: globalRule, - system: sysRule, + system: &sniffer{ + inner: sysRule, + repoDir: repoDir, + ref: opts.Ref, + runner: opts.Runner, + objcRule: objcRule, + }, }, filter, nil } @@ -461,7 +495,7 @@ func (c *composedResolver) ResolveDetail(path string) RuleDetail { return c.system.resolveDetail(path) } -func (c *composedResolver) matchProjectRuleDetail(pr *ProjectRule, path string, source string) *RuleDetail { +func (c *composedResolver) matchProjectRuleDetail(pr *ProjectRule, path, source string) *RuleDetail { entry := matchProjectRuleEntry(pr, path) if entry == nil { return nil diff --git a/internal/config/rules/system_rules.json b/internal/config/rules/system_rules.json index d26becb8b..1bef75a6b 100644 --- a/internal/config/rules/system_rules.json +++ b/internal/config/rules/system_rules.json @@ -33,6 +33,7 @@ "**/*.{tf,hcl,tfvars}": "terraform.md", "**/*.bicep": "bicep.md", "**/*.nix": "nix.md", - "**/*.{hs,lhs}": "haskell.md" + "**/*.{hs,lhs}": "haskell.md", + "**/*.m": "matlab.md" } } diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 4eec0b623..f92e88e0a 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -112,6 +112,7 @@ func TestResolve_DefaultRules(t *testing.T) { {"service.proto", "Wire Compatibility"}, {"src/Main.hs", "Partial Functions"}, {"examples/Tutorial.lhs", "Partial Functions"}, + {"Models/main.m", "Indexing, Shapes, and Implicit Expansion"}, } for _, tt := range tests { @@ -136,7 +137,6 @@ func TestResolve_FallbackToDefault(t *testing.T) { "docs/architecture.txt", "Makefile", "ios/ViewController.swift", - "ios/ViewController.m", } for _, path := range paths { @@ -235,7 +235,7 @@ func truncate(s string, maxLen int) string { func TestNewResolver_DefaultOnly(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) @@ -247,7 +247,7 @@ func TestNewResolver_DefaultOnly(t *testing.T) { } func TestNewResolver_ProjectFileMissing(t *testing.T) { - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver should not fail when project rule is missing: %v", err) @@ -270,7 +270,7 @@ func TestNewResolver_ProjectRuleHighestPriority(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -311,7 +311,7 @@ func TestNewResolver_ProjectRuleFirstMatchWinsWithinFile(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -333,7 +333,7 @@ func TestNewResolver_ProjectRuleFallsBackToSystem(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -352,7 +352,7 @@ func TestNewResolver_CustomRuleOverridesDefault(t *testing.T) { t.Fatalf("write custom rule: %v", err) } - resolver, _, err := NewResolver(t.TempDir(), customPath) + resolver, _, err := NewResolver(t.TempDir(), customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -381,7 +381,7 @@ func TestNewResolver_EmptyRuleSkippedAndFallsBack(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -414,7 +414,7 @@ func TestNewResolver_EmptyRuleMergeSystemRuleReturnsSystemOnly(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -453,7 +453,7 @@ func TestNewResolver_ProjectRuleReplacesSystemRuleByDefault(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -483,7 +483,7 @@ func TestNewResolver_ProjectRuleMergesSystemRule(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -533,7 +533,7 @@ func TestNewResolver_MergeSystemRuleKeepsRulePriority(t *testing.T) { t.Fatalf("write custom rule: %v", err) } - resolver, _, err := NewResolver(repoDir, customPath) + resolver, _, err := NewResolver(repoDir, customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -576,7 +576,7 @@ func TestNewResolver_CustomOverridesProject(t *testing.T) { t.Fatalf("write rule.json: %v", err) } - resolver, _, err := NewResolver(repoDir, customPath) + resolver, _, err := NewResolver(repoDir, customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -610,7 +610,7 @@ func TestNewResolver_ProjectFileMalformed(t *testing.T) { t.Fatalf("write: %v", err) } - _, _, err := NewResolver(dir, "") + _, _, err := NewResolver(dir, "", ResolverOptions{}) if err == nil { t.Errorf("expected error for malformed project rule.json") } @@ -697,7 +697,7 @@ func TestNewResolver_FileFilterMerged(t *testing.T) { t.Fatalf("write: %v", err) } - _, filter, err := NewResolver(repoDir, "") + _, filter, err := NewResolver(repoDir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -716,7 +716,7 @@ func TestNewResolver_FileFilterMerged(t *testing.T) { } func TestNewResolver_FileFilterNilWhenEmpty(t *testing.T) { - _, filter, err := NewResolver(t.TempDir(), "") + _, filter, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -743,7 +743,7 @@ func TestNewResolver_FileFilterPriorityOverride(t *testing.T) { t.Fatalf("write: %v", err) } - _, filter, err := NewResolver(repoDir, customPath) + _, filter, err := NewResolver(repoDir, customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -787,7 +787,7 @@ func TestNewResolver_FileFilterFallsToProject(t *testing.T) { t.Fatalf("write: %v", err) } - _, filter, err := NewResolver(repoDir, customPath) + _, filter, err := NewResolver(repoDir, customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -800,7 +800,7 @@ func TestNewResolver_FileFilterFallsToProject(t *testing.T) { } func TestResolveDetail_SystemDefault(t *testing.T) { - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -820,7 +820,7 @@ func TestResolveDetail_SystemDefault(t *testing.T) { func TestResolveDetail_SystemPatternMatch(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -840,7 +840,7 @@ func TestResolveDetail_SystemPatternMatch(t *testing.T) { func TestResolveDetail_SystemPrismaPatternMatch(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -864,7 +864,7 @@ func TestResolveDetail_SystemPrismaPatternMatch(t *testing.T) { func TestResolveDetail_SystemGoPatternMatch(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -895,7 +895,7 @@ func TestResolveDetail_SystemGoPatternMatch(t *testing.T) { func TestResolveDetail_SystemPHPPatternMatch(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -926,7 +926,7 @@ func TestResolveDetail_SystemPHPPatternMatch(t *testing.T) { func TestResolveDetail_SystemComposerPatternPrecedesJSON(t *testing.T) { t.Setenv("HOME", t.TempDir()) - resolver, _, err := NewResolver(t.TempDir(), "") + resolver, _, err := NewResolver(t.TempDir(), "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -966,7 +966,7 @@ func TestResolveDetail_ProjectOverridesSystem(t *testing.T) { t.Fatalf("write: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -1009,7 +1009,7 @@ func TestResolveDetail_MergeSystemRule(t *testing.T) { t.Fatalf("write: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -1056,7 +1056,7 @@ func TestResolveDetail_CustomOverridesAll(t *testing.T) { t.Fatalf("write: %v", err) } - resolver, _, err := NewResolver(repoDir, customPath) + resolver, _, err := NewResolver(repoDir, customPath, ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -1082,7 +1082,7 @@ func TestNewResolver_BraceExpansionInProjectRule(t *testing.T) { t.Fatalf("write: %v", err) } - resolver, _, err := NewResolver(dir, "") + resolver, _, err := NewResolver(dir, "", ResolverOptions{}) if err != nil { t.Fatalf("NewResolver: %v", err) } @@ -1508,6 +1508,15 @@ func TestResolveRuleEntries_GlobalRuleFileResolution(t *testing.T) { } } +// specialCaseRuleDocs lists rule_docs files loaded directly by Go code rather +// than referenced from system_rules.json's path_rule_map, so the orphan-file +// check in TestSystemRulesIntegrity must not flag them. +var specialCaseRuleDocs = map[string]bool{ + // Backs the MATLAB/Objective-C ".m" content sniff in ResolveWithContent; + // loaded via a dedicated ReadFile("rule_docs/objc.md") in LoadDefault. + "objc.md": true, +} + // referencedRuleFiles reads the embedded system_rules.json and returns the set of // rule_docs filenames it references (default_rule + every path_rule_map value). // A plain map decode is enough here: we only need the value set, not key order. @@ -1571,7 +1580,7 @@ func TestSystemRulesIntegrity(t *testing.T) { if e.IsDir() { continue } - if !refs[e.Name()] { + if !refs[e.Name()] && !specialCaseRuleDocs[e.Name()] { t.Errorf("rule_docs/%s is not referenced by system_rules.json (orphan file)", e.Name()) } } diff --git a/internal/scan/agent.go b/internal/scan/agent.go index 6eab5caf8..799bf780a 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -703,7 +703,7 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) (bool, st rule := "" if a.args.SystemRule != nil { - rule = a.args.SystemRule.Resolve(strings.ToLower(it.Path)) + rule = a.args.SystemRule.Resolve(it.Path) } planGuidance := a.maybeRunPlan(ctx, it, rule) diff --git a/pages/src/content/docs/en/review-rules.md b/pages/src/content/docs/en/review-rules.md index 1e15169d2..8ec415d62 100644 --- a/pages/src/content/docs/en/review-rules.md +++ b/pages/src/content/docs/en/review-rules.md @@ -177,11 +177,45 @@ matching order: | `**/*.jl` | `julia.md` — Julia source. | | `**/*.{tf,hcl,tfvars}` | `terraform.md` — Terraform / HCL. | | `**/*.bicep` | `bicep.md` — Bicep (Azure) templates. | +| `**/*.m` | `matlab.md` by default — see [Content sniffing for `.m` files](#content-sniffing-for-m-files) below. | | *(fallback)* | `default.md` | The resolved rule body becomes the `{{system_rule}}` placeholder in the plan and main task prompts. +### Content sniffing for `.m` files + +`.m` is shared by two unrelated languages — MATLAB and Objective-C — so path +matching alone can't tell them apart. Before falling back to `matlab.md` for +a `**/*.m` match, OCR peeks at the file's **first non-blank line**: + +| First line looks like | Rule doc used | +|---|---| +| `#import`, `#include`, `@interface`, `@implementation`, `@class`, or `@protocol` | `objc.md` | +| anything else (including no content available to sniff, e.g. a deleted file) | `matlab.md` | + +The content is read **at the ref under review**, not from your working tree: +`ocr review --from/--to` reads via `git show :` and `--commit` via +`git show :`, so the sniff is correct even when that ref isn't +checked out. Workspace reviews, `ocr scan`, and `ocr rules check` have no ref +and read the working tree, which is the thing they operate on. If the file +can't be read at all, resolution falls back to `matlab.md`. + +`objc.md` currently ships as a copy of the generic `default.md` checklist — +it's a placeholder in OCR's source +(`internal/config/rules/rule_docs/objc.md`) for a maintainer to fill in with +real Objective-C–specific guidance later; since it's compiled into the +binary via `go:embed`, changing it requires rebuilding OCR from source, not +just editing the file on disk. If you need Objective-C–specific guidance +today without rebuilding, use a project-level +[`.opencodereview/rule.json`](#rule-file-format-layers-1-3) entry matching +your `.m` paths (e.g. `ios/**/*.m`) — project rules are checked before the +system layer, so they take priority regardless of the sniff. + +`ocr rules check` reports when this sniff fired: the `Pattern` line reads +`**/*.m (sniffed: objc)` instead of the plain `**/*.m`, so you can tell at a +glance whether a given `.m` file resolved via the sniff or the default match. + ## Inspecting which rule wins: `ocr rules check` ```bash