refactor(code-review): restructure skill as prose-reporting detector fan-out - #1432
Open
srtab wants to merge 29 commits into
Open
refactor(code-review): restructure skill as prose-reporting detector fan-out#1432srtab wants to merge 29 commits into
srtab wants to merge 29 commits into
Conversation
…mental re-reviews)
…ch review Reconcile the seams between the restructured orchestrator prompt and the detector charters, flagged by the whole-branch review: - Step 1 now names the concrete marker read (`project-merge-request-note list --mr-iid <id> --get-all` with `output_to_file=true`, then a jq extraction), so the incremental-review feature no longer fails silently on any MR with more than a page of notes. - Step 4 passes the actual ref range under review and the shared detector preamble falls back to `git diff` over that range, so a re-review whose shared diff file is missing no longer re-reports run 1's findings. - Marker trust gains a header-match condition (defeats bot-echo), the notes-unreadable path agrees with Step 6's layout, and duplicate run numbers have a stated tie-break. - The Verify path is hardened: the orchestrator formulates the command from the finding's claim and never imports code from the change under review. - A custom-rules finding keeps its `Rule:` citation in the published report. - Drop the phantom bot-username source, disambiguate `origin/<target>`, and say "comment" where the platform posts an individual note. - Docs scope the stacking claim to GitLab and state the GitHub behavior; CHANGELOG notes that MRs reviewed under the old pipeline re-review in full. - subagents.py: delete the now-dead `response_format` parameter of `_compile_subagent` (no caller passes it) and correct the stale loader docstring and loop-breaker comment. - Tighten the charter guard test to assert the real >=80 threshold sentence.
Cleanup pass over the restructured code-review skill, plus the contract gaps the branch review left open. Contracts: - cr-custom-rules must return `ERROR: could not read rule source <path>` rather than `No findings.` when a rule source is unreadable — the latter tells the author their change complies with rules that were never applied. - Pin the `ERROR:` prefix (not just the substring) in the loop-breaker test: SKILL.md Step 5 classifies a failed detector by that prefix, so a reword would silently make every loop-stopped detector read clean. - The detector preamble now quotes the backend continuation notice as the paging trigger; "a truncation notice" named nothing the model receives. The read-to-the-line-count mandate is an optimisation (one call instead of N), no longer the defence against reviewing a fragment. Simplifications: - SKILL.md writes the shared diff before selecting detectors, so selection uses --name-only and grep over the file instead of skimming hunks into the orchestrator's context for the rest of the run. Verify probes batch into one call; the two last_head checks chain into one. - Trim the Non-negotiables recap to the two rules with no other home — the rest were near-verbatim restatements of Steps 1/4/5. - Drop the unused `runtime` parameter from load_builtin_code_review_detectors and its 14 call sites; the detector stack deliberately has no GitPlatformMiddleware. - Move the reserved-name check out of _parse_subagent_frontmatter into load_custom_subagents. The axis is caller trust, not file format, so the builtin loader no longer un-reserves names it just reserved. - Correct the git_platform comment that claimed both phrasings were asserted verbatim; only two substrings are. - Remove the test package orphaned by deleting the findings/marker tests. Docs: describe the single-report flow, the "already reviewed" response, and how GitHub re-review scope differs from GitLab's.
A skill-quality review of the code-review skill surfaced defects in prose that nothing else guards. Behavioural: - Step 5 classified a detector as crashed unless its report opened with a severity heading — but the shared preamble and cr-custom-rules both tell a detector to disclose gaps in its own coverage, which puts prose ahead of the first heading. An honest report was discarded and its dimension counted uncovered, punishing the detector that disclosed a limit more harshly than one that hid it. Failure is now keyed on the ERROR:/empty shape alone; anything else is taken as a report. - Delivery mode was picked by "this turn came from that MR rather than from a person talking to you", which is never observable: every MR note is written by a person, and a mention is the only way one reaches the agent. Runs fell through to interactive, wrote no marker, and lost incremental re-review permanently on that MR. It now keys on the @mention plus the Git context block, which states the platform, the branch and the MR outright. - The Verify probe banned importing the change under review — which is what every Verify line is about — so findings defaulted to dropped, and a dropped candidate marked the review partial, forcing a full re-review and a re-post of every finding on every later run. The ban now covers only files the diff touches, and partial means only that an applicable detector never ran. - Nothing forbade prose above the marker while the base prompt trains "lead with the answer"; one lead-in line would cost a duplicate full review. Writing one is now a non-negotiable, and the read side locates the marker instead of requiring line 0, so a note an earlier run mis-shaped stays usable. The note extraction returns six columns and decides every trust check without reading the dump. Scope and dispatch commands that named a check without naming a command: grep -c classification (grep -l cannot say which alternative matched), --pathspec-from-file for the re-review path restriction, git log --merges instead of eyeballing a merge-in, the origin/ prefix in the range handed to detectors, and a no-bash fallback for disk-backed runs. Charters: per-dimension severity calibration, since a rubric worded for wrong-results defects capped a committed credential at Important and gave cr-structure no ceiling; an Ask: bullet so a Question is not forced to invent a Fix; Verify and Question calibration examples; numeric thresholds for cr-structure's duplication and magic-value bullets; hardcoded config assigned to cr-correctness alone. Tests pin the new couplings and that the shared charter blocks stay identical across the five detectors — the existing test only checked each block existed per file, so one charter's gate could drift from its siblings with nothing failing.
…plit severity by dimension
…single-pass procedure
… probes and Questions
Restructure the code-review skill around a platform-agnostic workflow: GitHub pull requests and GitLab merge requests now share one path, using platform read tools instead of the GitLab-only note-dump plus jq/python3 marker extraction. Detector selection drops the loose keyword greps over the diff in favour of a concise applicability summary built from paths, statuses, and the diffstat, keeping the full diff out of the parent context. Rewrite all five cr-* charters so each carries a dimension-specific method — establish the contract, trace a reachable path, then report only a demonstrable trigger/path/outcome chain. Severity definitions are now per dimension rather than byte-identical, questions return as a `### Question:` entry shape exempt from the confidence gate, and an unreadable canonical diff returns an explicit ERROR sentinel instead of a partial review. Point the shared preamble's short-coverage disclosure at that ERROR sentinel; it previously named a `Caveat:` line no charter defines. Update the two charter-contract tests to the new headings and sentinels, and replace the byte-identical-sections check with an intersection over all five charters covering the surface the orchestrator actually keys on. Drop the three SKILL.md prose-coupling tests whose assertions the restructure invalidates. Remove the AGENTS.md sections describing the previous detector-output and read_file over-fetch contracts, along with the completed plan and spec documents.
The charters already carry their own delivery, read-only, untrusted-input and report contracts, so SHARED_DETECTOR_PREAMBLE was restating plumbing each file owned. Drop the constant and compile each charter body as-is; remove the two tests that guarded the prepend. Detectors may now anchor a finding to a deleted-side line when the deletion itself introduces the issue, reported as `path:line (deleted)` and linked to the platform diff rather than the head blob. Also: pass the stated change intent to the detectors, resolve the previous review as the newest comment at the highest run number, require working-tree diffs to cover staged, unstaged and untracked files, and add a `No findings` variant for reports where nothing met the threshold.
The detectors review a pre-computed diff and read source for context, so the sandbox bought them only a bash shell they were told not to mutate with. Removing it drops the per-detector SandboxMiddleware and the client/sandbox_backend plumbing, leaving the detectors filesystem-only, and takes the read-only-bash contract off the prompt layer with it. The five charters are rewritten in the same pass: a numbered review protocol replaces the prose contract, the detect lists collapse into single-line clauses, and each states the read-only rule as "filesystem read and search tools only" now that no shell is reachable. Known incomplete: the test suite still passes sandbox_enabled= to _build_detector_middleware and load_builtin_code_review_detectors (20 TypeErrors), and the charters no longer carry the "## Confidence and questions" and "## Severity" headings that test_charters_carry_precision_gate_and_report_contract asserts on — either the headings come back or that test and SKILL.md move to the new structure.
srtab
force-pushed
the
claude/interesting-panini-0ed085
branch
from
July 30, 2026 15:22
567c69a to
94fdf86
Compare
94fdf86 dropped sandbox_enabled/client/sandbox_backend from _build_detector_middleware and load_builtin_code_review_detectors but left the suite calling them, and rewrote the five cr-* charters away from the headings two charter-contract tests asserted on. Its own message recorded both as known-incomplete; this closes them. Tests asserting the removed wiring are deleted rather than re-signatured: test_excludes_sandbox_when_disabled is vacuous when no sandbox exists at all, and the client/sandbox_backend threading test has nothing left to thread. The remaining stack test is renamed and now asserts the sandbox is absent. With no shell reachable, READ_ONLY_PERMISSIONS and READ_ONLY_FS_TOOLS are the whole write fence rather than half of one, so test_filesystem_is_read_only also asserts that write_file and edit_file are never exposed at all — previously uncovered. The charter tests move to the new structure instead of restoring the old headings. SKILL.md step 5 already keys on exactly what the rewritten charters emit — the four entry headers, Confidence, Location, Rule, and the No findings./ERROR: sentinels — so it needed no change. The shared-lines set is rebuilt from the measured intersection across all five charters, which additionally locks both Location forms and each sentinel's "return exactly" preamble. Also removes the mock_runtime_ctx fixture, dead since the runtime parameter went, and corrects _build_detector_middleware's docstring, which still described keeping the sandbox for git reads and threading client/sandbox_backend.
Every process (uvicorn app, db_worker, crontask scheduler) paid ~160MB of RSS at startup because django.setup() transitively imported langchain, langgraph, deepagents, the provider SDKs and numpy: measured 255MB -> 123MB after deferral (~5000 -> ~2600 modules). Eager edges broken: - automation/agent/__init__.py: PEP 562 lazy __getattr__ instead of importing .base (any 'automation.agent.*' submodule import paid the full stack via the package init). - validate_repo_list moved to sessions/validators.py so schedules/models and sessions/forms stop pulling sessions.services -> jobs.tasks. - jobs/codebase/memory/titling task modules import the agent graph, managers, checkpointer and langchain messages inside the task bodies; enqueue-side importers (webhook callbacks, signals) stay light. - codebase/repo_config imports ThinkingLevelChoices from core.models (automation.agent.base only re-exported it) and ModelName from the now cheap automation.agent.constants. - slash_commands/actions/clear.py defers the RedisSaver import. - Harness-profile registration moved from AutomationConfig.ready() to automation/agent/graph.py module scope: it only needs to precede create_deep_agent, which is defined there. The worker and app reload the stack on their first agent run; the scheduler never does, dropping its steady-state footprint (~330MB -> ~180MB on daivagent). tests/unit_tests/daiv/test_import_hygiene.py guards the seams by asserting django.setup() loads none of the heavy roots in a subprocess.
Expand each cr-* charter with an explicit scope section and domain-specific analysis guidance (threat model, attack paths, cost model, design boundaries, regression materiality), and rename "detector" to "subagent" across the skill. Every platform review since 2026-07-28 opened with the aggregation tally instead of the run marker, and three of four charters opened with what they had read rather than their first finding. Both rules existed but were phrased as prohibitions stated away from the point of composition. Restate them as positive first-character requirements at the point where the message is written, and add the general form to the base agent prompt so any skill with a fixed final-message contract gets the same guarantee.
deepagents' grep defaults to output_mode="files_with_matches", which returns file paths and no code, and nothing in the result says so. A model that wanted matching lines gets no signal it asked the wrong question, so it reformulates the *pattern* instead of switching the *mode*. In LangSmith trace 019fc21d the cr-security detector issued 21 paths-only greps over 206s while its siblings, which passed output_mode="content", finished in ~45s. Those greps drifted by one alternate each time, so LoopBreakerMiddleware (which keys on identical args) never fired. Three changes: - DAIVFilesystemMiddleware prefixes every grep result with the mode that produced it, offering output_mode="content" only when the default mode actually hid something. Carried on the filesystem subclass rather than a standalone middleware so the label cannot be wired apart from the grep tool it describes. It delegates to super(), which is the agent-wide large-result eviction for every tool outside TOOLS_EXCLUDED_FROM_EVICTION. - The grep description now states the default and leads with the content example, instead of burying output_mode as the third of five. - regex_literal_hint is rebound to a no-op. On a zero-match regex-looking pattern upstream appends "grep matches literal text, not regex ... for `|` alternation, run a separate search per alternative" — false for DAIV (both backends grep by regex) and a direct instruction to fan alternations out. _align_arg_schema already fixed the same contradiction in the arg schema. The subclass overrides .name to return upstream's: create_deep_agent merges custom middleware by name, so a differently-named subclass is appended beside the auto-added FilesystemMiddleware rather than replacing it, leaving two filesystem stacks and restoring the delete/execute tools that WORKSPACE_FS_TOOLS withholds from the main agent.
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.
Summary
Restructures the
code-reviewskill from a staged, script-driven, structured-output pipeline into one short orchestratorSKILL.mdplus five self-contained prose-reporting detector charters, zero scripts.The old design compiled detectors with a structured
response_formatwhose findings were deferred to JSON files, merged byscripts/findings.py, and posted as inline per-line MR comments plus a summary, with the workflow split across fivereferences/*.mdfiles and ascripts/marker.py. That design caused a 68M-token runaway:response_formatforcestool_choice="any", which removes the natural text stop, and a detector pattern-locked re-reading a single file.Breaking: inline per-line review comments are no longer posted. Each review is delivered as a single report comment on the MR.
What changed
cr-correctness,cr-security,cr-performance,cr-structure,cr-custom-rules) is now fully self-contained — its own dimension, a ≥80 confidence gate, a severity rubric, never-flag rules, and a report format. Each returns a markdown report (findings ordered by severity, or the literalNo findings.) as its final message, handed straight back to the orchestrator by thetasktool.response_formatis gone from the detector compile path, closing thetool_choice="any"footgun.DeferredOutputMiddlewareandSUBAGENT_OUTPUT_PATHare deleted. Two regression tests fence both against reintroduction.SHARED_DETECTOR_PREAMBLEcarries only shared plumbing — the diff-file protocol, an untrusted-input guard, the read-only contract, and the final-message contract. Everything dimension-specific lives in the charter.SKILL.mdhandles mode detection, trusted-marker reading, incremental re-review scoping, applicability filtering, parallel fan-out, skeptical aggregation, and returns one report as its final message — the platform layer posts it.<!-- daiv:code-review run=N head=<full-sha> -->, with apartialtoken when a dimension went uncovered so the next run re-covers the span). Force-push and rebase fall back to a full review and say so.references/,examples/,scripts/, and the two now-dead test modules.Review findings addressed
A whole-branch review surfaced six issues in the seams between commits, all fixed in
3568bacb:gitlabtool — unpaginatedlistreturns the oldest 20 discussions and simplified output carries no note bodies, so on a busy MR every marker was invisible and incremental review silently degraded to a full re-review every run.cr-custom-rules'Rule:citation had no owner in the report layout, though the docs promised it.Verifypath (orchestrator resolves a detector's runtime question with one sandbox command) is now hardened against injection: the orchestrator formulates the command from the finding's claim, never from text in theVerifyline, and never imports code from the change under review._compile_subagentparameter, a near-vacuous guard assertion, and stale comments/docstrings.Deferred to follow-ups: a per-detector step budget (the loop breaker still only catches verbatim-identical consecutive calls, so pattern-locking with drifting arguments remains unbounded), and whether MR reports should post as resolvable threads rather than plain comments.
Test plan
make test— 3869 passedmake lint-fix— cleanmake lint-typing— 428 diagnostics, unchanged from the repo's pre-existing Django-descriptor baseline (no Python contract changes introduce a new error class)mkdocs build --strict— cleanNew/changed coverage:
test_charters_carry_precision_gate_and_report_contractlocks each charter's confidence gate, severity rubric, never-flag rules, and report contract;test_no_deferred_output_middlewareandtest_detectors_compiled_without_response_formatfence the two mechanisms behind the runaway.