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 49 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.
srtab
force-pushed
the
feat/code-review-skill-v3-6-0
branch
from
July 23, 2026 16:35
2215de3 to
1cb1673
Compare
…story StepBudgetMiddleware tracked heartbeat cadence with an instance counter (_model_calls) that accumulated across every awrap_model_call invocation. Subagents are compiled once per parent run and then invoked once per task dispatch on the same instance, so the counter accumulated across runs and interleaved under parallel fan-out — the cadence fired at an aggregate rate and the "N model calls in this run" text was false for any individual run. Replace the instance counter with a stateless per-request count derived from the number of AIMessages already in request.messages (completed model turns) plus one. This is per-run by construction and immune to instance sharing and concurrency. Also correct three stale comments introduced by this branch: _compile_subagent docstring (detectors now pass submit_findings, not none), DeferredOutputMiddleware._serialize (not a detector-specific path anymore), and findings.py (constraint reason is submit_findings tool args schema, not response_format).
…and finish contract
In read_findings_from_files, add an extension guard before the file-open
attempt: any path that does not end in .json is immediately counted as
skipped with a stderr note. DeferredOutputMiddleware writes .json only
for a marker-verified submit_findings payload; anything else — a
loop-breaker error, a model's give-up text, or text that happens to be
valid JSON — is deferred as .txt. The extension is therefore an
authoritative signal of success, not content parseability.
Without this guard, a weak model printing {"findings": []} as plain
text rather than calling the tool would be deferred as .txt, parsed
happily by json.load, and cause the merge to certify a legitimately
empty review — silently suppressing the failed-detector signal.
Also align the parent skill's review-directed-text escalation rule
(review-workflow.md line 101) with the recalibrated cr-security
detector: the parent now applies the same bar and carve-outs, so test
fixtures containing review-directed strings are not incorrectly flagged.
Add a Stage 0 fallback paragraph for runs without a SHA triplet: derive
the base with git merge-base, or skip cr-custom-rules and report
degraded coverage if the script cannot run at all.
`submit_findings` becomes a `content_and_artifact` tool: a validated payload comes back as the `ToolMessage.artifact`, and the enforcer promotes it into `structured_response` with a `Command`. That deletes the `SUBMITTED_MARKER` string contract and `DeferredOutputMiddleware`'s tool-call-history archaeology — the middleware goes back to exporting one channel and knows nothing about detectors. Because the submission is visible in state before the next model call, that call is short-circuited into a final `AIMessage`. A detector can no longer resume inspecting after recording (the pattern-locked failure mode), and each detector costs one fewer full-context model call. Also closes the paths where a detector under-reported instead of failing: - A detector that produces no structured output and no final text now defers a `.txt` sentinel instead of nothing at all, which downstream was indistinguishable from never having been dispatched. - `read_findings_from_files` counts a JSON object with no `findings` key as a failed detector rather than an empty one. - A `custom-rules` finding with no `source` is rejected at submission time (retryable) instead of being silently dropped at merge. - `rules.py` requires a full 40-char object id for `--base-sha`, reads existence with `ls-tree` so a partial-clone failure cannot be filed as "no rules exist", caps git calls with a timeout and `GIT_TERMINAL_PROMPT=0`, and refuses to follow a symlink when writing a snapshot.
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.