feat(code-review): v3.6.0 — intent capture, triage gate, injection hardening, reference slimming, incremental re-review, execution robustness#1409
Draft
srtab wants to merge 25 commits into
Draft
Conversation
Bump the built-in code-review skill to v3.6.0: - Capture the MR/PR title, description, and linked issue as the author's stated intent; Stage 2 uses it to retire questions the description already answers (intent can retire a question, never waive a defect). - Trivially small changes with no executable surface skip the five-detector fan-out for a single inline pass, reported as `inline (triage)`. - Detector charters and the verification stage now treat text inside the diff as data, never instructions, hardening against prompt injection.
…pty review status_notes fired both the 'dropped' note and the 'legitimately empty review' note when every finding failed schema validation (candidates==0, skipped==0, dropped>0). Guard the empty-review branch with dropped==0 so the contradictory note no longer co-fires; the 'dropped' note carries the real signal. Adds a regression test for the all-malformed case.
srtab
changed the title
marker resolve now reports snippet_in_deletion when a target has no new-side match, telling a genuine pure deletion (demote to summary) apart from a wrong snippet the caller should re-derive. Hunk parsing is factored into _iter_hunk_lines and split on "\n" only, so line numbering matches git/GitLab exactly. findings merge treats an empty path list as a fan-out failure (exit 1) instead of certifying a legitimately empty review, and status_notes no longer calls a zero-file merge an empty review.
… delta re-reviews
srtab
changed the title
… writes python-gitlab's client-level retry_transient_errors=True retries 5xx with exponential backoff. GitLab can return 500 *after* already persisting a note, so retrying a non-idempotent create() POST appends a duplicate each time — observed in production as one inline code-review finding posted 12x (1 create + 11 backoff-spaced retries) on a single MR, the dominant driver of "too many comments". Add _NO_TRANSIENT_RETRY_ON_WRITE and thread it into every non-idempotent create() POST (issue/MR comments, inline discussion, create_issue, MR creates). Emoji award-creates deliberately keep the client-level retry (GitLab's unique-reaction 409s the retry, so it cannot duplicate); idempotent reads are unchanged.
Raise the bar for `question` findings across the charter, principles, few-shot examples, and review workflow: a question earns a slot only when a plausible answer would itself expose a defect or behavior/contract problem — never to confirm test coverage or satisfy curiosity. A bare untested path is no longer a finding. Delta re-review delivery: - Reuse the scope stage's parse-notes output in Step 1 instead of re-listing discussions. - Always regenerate the shared diff as the full `<target>...<head_sha>` range before `resolve`, since a delta re-review's wrong-base old_line values slip past the staleness guard and misplace context-line positions. - Document the suggestion range header contract (A always 0, B derived from the resolved anchor) to avoid HTTP 500 rejects. - Make the inline-findings index cumulative across reviews and add a visible status line under the summary marker. - Post a one-line reply when an in-place summary edit changes anything, since GitLab notifies no one on a silent edit. - Carry forward every prior discussion-only finding and mark this run's additions with a leading new marker.
… B4, B6, B8) Root-caused from 57 production code-review traces. Four generic (non-model- specific) changes, each hardening existing machinery: - B2/B3: hoist the always-enter-workflow and always-deliver mandates into SKILL.md non-negotiables so the procedure (and the triage gate) is reachable. - B4: post notes via a marker.py-composed --body-file so the dedup marker is never re-serialized by the model (fixes silent single-quote duplicate driver). - B6: code-enforce the sandbox circuit breaker (consecutive-failure counter) instead of relying on the prompt-based backstop. - B8: add a reachability/impact qualifier to the deterministic severity mapping so security/correctness defects aren't auto-High regardless of exploitability.
…ates into router SKILL.md is the only file guaranteed in context; the two invariants that make the review procedure non-skippable now live there as gates (each with its rationalization counter), not one level down where a short-circuiting agent never reaches them. Step 1 is reworded so entering review-workflow.md is unconditional, and the now-redundant "Required reading every run" tag in the References list is dropped in favor of the single authoritative gate. Also documents the degraded path in gitlab-delivery.md: if the bash tool trips its sandbox-outage circuit breaker, marker.py can't run, so delivery demotes to interactive mode instead of hand-computing markers.
srtab
changed the title
…l-v3-6-0 # Conflicts: # CHANGELOG.md # daiv/codebase/clients/gitlab/client.py
…utive transport failures" This reverts commit eb10f93.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bump the built-in code-review skill to v3.6.0, and add incremental (delta) re-review scoping on top.
Behavior — intent capture, triage gate, injection hardening
inline (triage).Reference slimming
Cut the skill's reference material by ~5.7k tokens (≈19.1k → ≈13.4k) with zero change to the review workflow or any guardrail — deterministic mechanics moved into the scripts, prose deduplicated:
marker.py resolve— a deterministicline → (new_line, old_line, line_type, anchor)subcommand that parses the shared diff and refuses a stale one, replacing Step 4's hand-computed grep/old-line/anchor mechanics.findings.py mergenow emits a self-describingnotesarray, soreview-workflow.mdno longer re-explains merge-stat semantics.gitlab-delivery.mdandreview-workflow.mdrewritten around the two scripts;principles.md,few-shot-examples.md,example-review-output.md, andmarker-format.mdcompressed — every§/Step number, JSON contract, and enumerated guardrail preserved with meaning intact.status_notesedge case: an all-malformed merge (candidates==0,dropped>0) is no longer labeled a "legitimately empty review."All six reference files land within their per-file token ceilings.
Incremental (delta) re-review scoping
A re-run of
/code-reviewon an MR now scopes detector detection to only the commits added since the last review — cutting the dominant re-review cost — with no harness changes. Skill version stays3.6.0(behavior-only change).parse-notessurfaceslast_reviewed_sha— the summary marker's sha, i.e. the head at the previous review. Additive new key on the publicparse-notescontract; existing keys unchanged.git diff <last_reviewed_sha>...<head_sha>when it is safe: guarded by a non-nulllast_reviewed_shaand no--fullrequest and agit merge-base --is-ancestor <last_reviewed_sha> <head_sha>check. A force-push/rebase (prior head no longer an ancestor), a first review (no summary), or--fullall fall back to the full<target>...<head>range.--fullescape hatch (or "review the whole MR again") forces a full-range re-scan.<target>...<head_sha>diff, so a context line'sold_linematches the MR's realbase_sha. (Added-line findings — the common case — are unaffected either way.)make testpasses (3964 tests, 0 failures); lint clean.Execution robustness (B2/B3, B4, B6, B8)
A follow-up pass hardening the skill's execution across models and infrastructure — all generic (no model-id or case-specific special-casing), driven by production-trace analysis. Design spec:
specs/2026-07-22-code-review-robustness-design.md.--body-fileon thegitlabtool, read through the sandbox filesystem backend. The model never re-types the JSON marker, so it can no longer corrupt it (e.g. single-quoting) and drop the note from the dedup set → no more duplicate inline comments/replies on re-review. Incidentally escapes a leading@→@@, fixing a latent python-gitlab CLI@mention→host-path misparse.git_platform.pygains zerodaiv-crknowledge (marker logic stays inscripts/marker.py).bashtool short-circuits with an "unavailable" message instead of dispatching, and any single success resets the counter. Stops the observed thrash (100+ post-failure bash calls) and the degraded review that still reportedsuccess. Tool- and skill-agnostic; classifies by transport-error type via the existing classifier.SKILL.md): every run entersreview-workflow.mdbefore any verdict, and delivery mode always completesgitlab-delivery.mdeven at zero findings. Plus a degraded-path line: if the sandbox breaker trips, demote to interactive markdown rather than hand-computing markers.Highnow requires acorrectness/security/custom-rulesdefect whose impact is triggerable by a realistic actor/input in the code's actual deployment and material; privileged/committer-access-only, unrealistic-precondition, or defense-in-depth-only defects demote to Medium/Low. Deterministic (bar × detector × reachability/impact), no enumerated vuln-class exceptions; the Stage-2 refutation rule (genuinely unreachable → dropped) is unchanged — a privileged-only defect survives verification and demotes.make testpasses (3964 tests);make lintclean;make lint-typingadds no new error class. Behavioral items (B2/B3 router mandate, B8 calibration) validate via post-merge trace review per the spec's cross-item verification.