fix: remove redundant review rules and fix strategy/criteria mismatch - #348
Merged
Conversation
Several review cleanups driven by the observation that the `add_documentation_rules` step review was taking forever: - job.yml: change `add_documentation_rules` review from `strategy: individual` to `strategy: matches_together`. Two of its three criteria (Documentation Covered, Efficient Rule Count) are project-wide and cross-cutting, so running them per-file forced each reviewer to redo the full project scan N times. - job.yml: drop the `apply_rule` "Valid Configuration" criterion — the `deepreview` DeepSchema already enforces structural/schema validation via `json_schema_path`. - job.yml: collapse the duplicated rule-YAML blocks in `add_deepwork_native_reviews`. The step previously showed the same two rules twice (once as a file-ref reference example, once as the inline version to emit) and told the agent to consult both, creating a contradiction about which form to use. - Delete `src/deepwork/standard_jobs/deepwork_reviews/.deepreview` (`job_definition_review` rule): fully redundant with the `job_yml` DeepSchema which covers the same files and criteria. - Delete `job_schema_instruction_compatibility` from the top-level `.deepreview`: its whole purpose was verifying job.yml files conform to `job.schema.json`, which is exactly what the `job_yml` DeepSchema's `json_schema_path` already enforces. - Add `review-strategy-matches-criteria-scope` requirement to the `job_yml` DeepSchema so this class of bug is caught going forward: review `strategy: individual` must only be used when every criterion can be evaluated from a single file in isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Discourages the sentinel-placeholder-file workaround (e.g., writing a NONE.md file when the real result is "nothing to report") used to satisfy required file_path outputs that would otherwise be empty. The fix is to use type: string for outputs whose list can legitimately be empty, since string outputs can naturally carry a narrative "no items found" message. Kept as SHOULD so genuine filesystem-artifact needs (manifests, etc.) can still opt into a sentinel, but the common "required output can't be empty" workaround is explicitly not a sufficient reason. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The step previously directed the agent to write a .deepwork/tmp/migrated_skills/NONE.md sentinel file as a workaround for "required file_path output can't be empty" — exactly the anti-pattern the new no-sentinel-files DeepSchema requirement added in a4dc74c prohibits. Verified the MCP validation accepts empty lists for required file_path outputs (no minimum-length check in src/deepwork/jobs/mcp/tools.py), so the sentinel workaround was never necessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous consolidation commit (1994f94) removed the duplicated rule-YAML blocks but also removed the pointers to plugins/claude/example_reviews/*.md, which contain the full detailed prompts that the inline YAML condenses. Those pointers are legitimately useful — they tell an agent writing condensed inline versions where to read for tone, intent, and checklist context. Restored as a "Reference material" section that clearly resolves the original inline-vs-file-based contradiction: always emit the inline form (target projects don't have the example files), and use the example files only as reference when deciding what the condensed inline text should say. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous wording forbade any cross-file reasoning from `strategy: individual`, which would disqualify legitimate cases like a Python code review rule where the reviewer may need to consult other project files (imports, references, related code, conventions) while evaluating a single changed file. Reworded so the distinction is about whether the reviewer needs to look at the OTHER CHANGED FILES in the same review set, not whether they need to look at any other file at all. Individual is appropriate when focus is heavily on each file and the reviewer is unlikely to need to see the other changed files; matches_together is for criteria that reason about the set as a whole (coverage, deduplication, consistency across the set). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restructured the requirement to use SHOULD/MAY/MUST NOT explicitly, matching the RFC 2119 style of the other requirements in this schema. The substantive guidance is unchanged: individual is preferred when focus is on each file and the reviewer is unlikely to need to see the other changed files (consulting unchanged project files is explicitly allowed as MAY); matches_together is preferred when changed files need to be looked at alongside each other; and individual is a hard MUST NOT when any criterion is a property of the set of changed files as a whole. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The meta-DeepSchema governing deepschema files previously only matched .deepwork/schemas/*/deepschema.yml and **/.deepschema.*.yml, which missed the project's own standard schemas in src/deepwork/standard_schemas/ — they were exempt from the rules they define for users. Consolidated both hard-coded directory matchers into a single generic **/deepschema.yml pattern that: - Covers .deepwork/schemas/*/deepschema.yml (user-created named schemas) - Covers src/deepwork/standard_schemas/*/deepschema.yml (built-in schemas) - Covers any other location a user might put a deepschema.yml - Self-matches the meta-schema itself, which is fine — it conforms to its own requirements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Several review cleanups driven by the observation that the
add_documentation_rulesstep review was taking forever to run:add_documentation_rulesreview fromstrategy: individualtostrategy: matches_together. Two of its three criteria (Documentation Covered, Efficient Rule Count) are project-wide and cross-cutting, so running them per-file forced each spawned reviewer to redo the same full project scan N times.apply_rule"Valid Configuration" criterion. ThedeepreviewDeepSchema already enforces structural/schema validation viajson_schema_path.add_deepwork_native_reviewspreviously showed the same two rules twice (once as a file-ref reference example, once as the inline version to emit) with contradictory guidance about which form to use. Collapsed to a single copy.src/deepwork/standard_jobs/deepwork_reviews/.deepreview(job_definition_reviewrule): fully redundant with thejob_ymlDeepSchema which already targets the same files and covers the same structural/coherence checks.job_schema_instruction_compatibilityfrom the top-level.deepreview: its entire purpose was verifyingjob.ymlfiles conform tojob.schema.json, which is exactly what thejob_ymlDeepSchema'sjson_schema_pathalready does.review-strategy-matches-criteria-scoperequirement to thejob_ymlDeepSchema so this class of bug is caught going forward: reviewstrategy: individualmust only be used when every criterion can be evaluated from a single file in isolation. Cross-file / set-level criteria must usematches_together.Test plan
python3 -c "import yaml; yaml.safe_load(...)")uv run pytest tests/unit/review/— 236 passed)add_documentation_rulesstep review completes in reasonable time on next discover_rules run🤖 Generated with Claude Code