morningman commented on PR #12: URL: https://github.com/apache/doris-skills/pull/12#issuecomment-5659248712
@shuke987 Thanks — both are real gaps, and both are addressed in `49ae610`. **1. Category-aware floor (the documented rule wins; the verifier now enforces it).** The intended rule is the documented one: only regressions in specified categories are floored at Major. The verifier now knows the categories instead of flooring every `Regression: yes`: - Every finding carries a mandatory `Category` / `类别` line from a closed vocabulary ([`verify-review-docs.py` L44-L60](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/scripts/verify-review-docs.py#L44-L60), [`doc-templates.md` "Category"](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/references/doc-templates.md#L63-L85)). The class names the *consequence*; the domain (concurrency, lifecycle, …) goes in a parenthesis after it, e.g. `functional-loss (lifecycle)`. An unknown value is rejected rather than defaulted, so a typo cannot slip past the floor; EN and ZH must agree. - **Floored (regression ⇒ at least Major):** `functional-bug`, `functional-loss`, `data-error`, `resource-leak`, `performance`. - **Judged normally:** `observability`, `test-coverage`, `wording`, `maintainability` — but a `Regression: yes` rated below Major must carry a *Severity rationale* paragraph saying why the behaviour change is acceptable; the verifier checks that it exists ([L264-L275](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/scripts/verify-review-docs.py#L264-L275)). Your example (debug log lost some detail → `observability` / `Regression: yes` / Minor) now verifies with such a paragraph. - The PASS gate is separate, as you suggested: the poster refuses a *floored* regression (only possible if the two scripts ever disagree) and otherwise still posts, but refuses when a non-floored regression would go undisclosed — the receipt must name each one in the notes ([`post-pass-comment.sh` L104-L142](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/scripts/post-pass-comment.sh#L104-L142)). The `doris-repo-review/v1` comment schema is unchanged; the verifier JSON gains `floored_regressions`. **2. Regression attribution uses `MERGE_BASE`.** You are right that `BASE_SHA` is the target-branch tip. Regression evidence is now `git show $MERGE_BASE:<path>` everywhere — SKILL.md premise checks and step 6, the subagent preamble / D1 / severity-challenge prompts (`{MERGE_BASE}` is a new placeholder), the ledger skeletons, and the template ([`doc-templates.md` "Regression"](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/references/doc-templates.md#L86-L105), [SKILL.md step 6](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/SKILL.md#L383-L398)). `BASE_SHA` keeps its two jobs: the diff range and the commit the receipt binds to. `prepare-review-context.sh` records `TARGET_AHEAD` (target-branch commits since the merge base) in `meta.env` and warns when it is non-zero ([L157-L160](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/scripts/prepare-r eview-context.sh#L157-L160)); the case is also listed under common traps ([SKILL.md](https://github.com/apache/doris-skills/blob/49ae61080fa1fcfe2789df8d5d85aa8cf985a56f/skills/doris-repo-review/SKILL.md#L652-L657)). Compatibility with a target branch that has moved on is called out as its own concern, never as `Regression: yes`. For what it is worth, the real review of apache/doris#67916 already had this shape: its header shows `BASE_SHA` ≠ `MERGE_BASE`, with 5 target-branch commits in between. **Tests.** `verify/repo-review/run.sh`: 63 PASS (was 47) — each floored category rejected at Minor, each non-floored category accepted at Minor with a rationale and rejected without one, the parenthetical domain note, spelling variants, missing / unknown / disagreeing categories, and the poster's disclosure gate. Also replayed against the real #67916 documents: they verify after relabelling with the closed vocabulary; flipping a `test-coverage` finding to `Regression: yes` is rejected without a rationale and accepted with one; flipping a `functional-bug` finding to `Regression: yes` at Minor is rejected. Compatibility note: documents from the previous template now also need the `Category` line, same as the `Regression` line this PR already required. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
