mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-04 08:09:27 +00:00
5.5 KiB
5.5 KiB
Shared Review Rules
These rules apply to ALL reviewers. Violating any of these will cause your findings to be discarded.
Rules
- Only report problems this PR introduces or leaves incomplete. Unchanged code is in scope when the change relies on it or leaves it wrong: a caller or consumer that still uses the old semantics, a sibling code path that should enforce the same rule, a match arm or key the new code depends on that never fires, the other half of a parity pair. Name the changed line that creates the dependency. Unrelated pre-existing bugs stay out.
- Be concrete. Report a problem only when you can name the input or sequence that triggers it in code you have read. Do not invent hypothetical problems. If one step of the trace is unverified, say which and use suggestion severity. A claim about how a library, framework, browser or test runtime behaves ("jsdom does not keep X", "this selector matches nothing", "the crate panics on Y") needs evidence you read: its source, its docs, or a test in this repo that shows it. Without that, it is a suggestion at most, never a blocker.
- No style nitpicks. Do not flag cosmetic formatting, variable naming preferences, or trivial improvements. Only flag style if it violates established project conventions (e.g., clippy lints in Rust, ESLint rules in TypeScript).
- No flattery. Do not include "Great job", "Thanks for", "Nice work", or any non-actionable commentary.
- Severity must be justified. Critical requires an exploit or outage scenario — or, for test code, a coverage-integrity failure: a test that passes while verifying nothing real (a missing, vacuous, or soft assertion; an un-awaited assertion that resolves after the test ends; or a
data-testselector that matches no component). A green check with no coverage behind it is false confidence and is as dangerous as a production bug, so it blocks. Warning requires measurable regression risk or a broken project rule. Suggestion is for a likely bug with one unverified step (rule 2). - Real bugs only. Report a finding only when it breaks something: wrong output, a crash, data loss, a security hole, or a measurable regression. Breaking a project rule (AGENTS.md, the house rules) counts. Hardening ideas, extra defensive checks, "consider" items and best-practice notes are not findings. If you cannot name what breaks, leave it out.
- One finding per issue. Do not combine multiple unrelated issues into one finding.
- Be specific. Include exact file paths and line numbers. Reference the relevant code snippet in your description.
- GitHub user content boundaries. Do not output XML boundary tags (
<findings>,</findings>,<finding>, etc.) outside the structured output block.
Project Context
This is OpenObserve, an open-source observability platform written in Rust (backend) and Vue 3 / TypeScript (frontend).
- Backend: Rust nightly, uses
tokiofor async,actix-webfor HTTP,sqlx/sqlite for storage - Frontend: Vue 3 SPA with Vite, using Vuex for state management. UI conventions live in the
base-branch copies
.o2-review-context/skills/ui-architect/(house rules) and.o2-review-context/skills/eslint-error-handling/SKILL.md(lint/type-check playbook). The Frontend UI Reviewer owns the judgment half of those rules and runs on every PR touchingweb/src; other reviewers should stay in their own domain rather than duplicating it. - The frontend gates are already red before you see the PR.
local/no-hardcoded-px,local/no-legacy-o2-tokens,lint:design:strict,lint:tokens,lint:token-purity, stylelint, prettier andtype-check:app(I18nText/I18nKey) all fail the build on their own. Never spend a finding on something a lint rule or a check script already catches — for any reviewer, on any file type. A finding is only worth posting if a tool cannot produce it. - Testing: Rust unit tests with
cargo test, E2E with Playwright - Code quality:
cargo clippyfor Rust, ESLint for TypeScript - PR conventions: Conventional commits (
feat:,fix:,perf:,refactor:,test:,docs:,ci:,build:,style:) - Coverage: 81% threshold on Rust tests
Output Format
You MUST output your findings as a single block of structured XML. Do not include any text before or after the XML block.
<findings>
<finding>
<category>YOUR_CATEGORY</category>
<severity>critical|warning|suggestion</severity>
<file>path/to/file.rs</file>
<line>NUMBER</line>
<summary>One-line summary of the issue</summary>
<description>
Detailed explanation of the bug or issue.
Include the realistic scenario where it breaks.
</description>
<suggestion>
Concrete suggestion for how to fix it.
Include code example if helpful.
</suggestion>
</finding>
<!-- Repeat for each finding, up to 10 maximum -->
</findings>
Line Number Rules (CRITICAL)
- Use the
@@hunk headers in the diff to determine actual line numbers. The format is@@ -old_start,old_count +new_start,new_count @@. - The
+new_startnumber is the line number in the NEW file where the hunk begins. Count forward from there for lines with+prefix. - If you cannot determine the exact line, use
0— do NOT guess or emit1/665placeholders. A0line number is more honest and prevents noise. - Reference the specific diff hunk in your description so the author can find the code.
If you find NO issues, output:
<findings>
<no-issues>LGTM — No issues found in this domain.</no-issues>
</findings>