fix: remove redundant review rules and fix strategy/criteria mismatch (#348)

* fix: remove redundant review rules and fix strategy/criteria mismatch

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>

* feat: add no-sentinel-files SHOULD requirement to job_yml DeepSchema

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>

* fix: replace migrate_existing_skills sentinel file with empty list

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>

* fix: restore reference to example review prompt files

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>

* fix: soften review-strategy-matches-criteria-scope wording

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>

* fix: use RFC 2119 keywords in review-strategy-matches-criteria-scope

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>

* feat: broaden deepschema matchers to **/deepschema.yml

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>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: nhorton

.deepreview

@@ -369,46 +369,6 @@ shell_code_review:
         section markers) still accurate after the changes? Flag any comments
         that describe behavior that no longer matches the code.
 
-job_schema_instruction_compatibility:
-  description: "Verify all standard and library job.yml definitions and templates are compatible with the job schema."
-  match:
-    include:
-      - "src/deepwork/jobs/job.schema.json"
-      - "src/deepwork/standard_jobs/*/job.yml"
-      - "src/deepwork/standard_jobs/deepwork_jobs/templates/job.yml.*"  # Templates users copy — must match schema
-      - "library/jobs/*/job.yml"
-  review:
-    strategy: matches_together
-    additional_context:
-      unchanged_matching_files: true
-    instructions: |
-      When the job schema, standard job definitions, or library job definitions
-      change, verify they are still compatible with each other.
-
-      Read src/deepwork/jobs/job.schema.json to understand the current schema.
-      Then read each job.yml (standard and library) and check:
-
-      1. **Field references**: Every field name referenced in inline step
-         instructions must exist in the schema at the correct level.
-         Pay special attention to step_arguments vs workflow vs step fields.
-
-      2. **Required vs optional**: If instructions say a field is required,
-         verify the schema agrees. If instructions say a field is optional,
-         verify the schema doesn't require it.
-
-      3. **Schema structure**: Any YAML examples shown in inline instructions
-         must match the schema's property names and nesting.
-
-      4. **Terminology consistency**: Instructions should use the same field
-         names as the schema (e.g., if the schema uses
-         "common_job_info_provided_to_all_steps_at_runtime", instructions
-         should not call it "description" or "job_description").
-
-      Output Format:
-      - PASS: All job definitions are compatible with the schema.
-      - FAIL: Incompatibilities found. List each with the file path, line
-        reference, the incompatible content, and what the schema actually says.
-
 agents_md_claude_md_symlink:
   description: "Ensure every AGENTS.md file has a sibling CLAUDE.md symlink pointing to it, because Claude Code reads CLAUDE.md but ignores AGENTS.md."
   match:

src/deepwork/standard_jobs/deepwork_reviews/.deepreview

@@ -49,59 +49,25 @@ workflows:
 
           Ensure the project's top-level `.deepreview` file contains the two built-in DeepWork review rules: `suggest_new_reviews` and `prompt_best_practices`. These are general-purpose rules that benefit any project using DeepWork Reviews.
 
-          ## Task
-
-          ### 1. Read the example review instruction files
+          ## Reference material
 
-          Read these files from the DeepWork plugin:
+          The DeepWork plugin ships full, detailed versions of these two review prompts at:
           - `plugins/claude/example_reviews/prompt_best_practices.md`
           - `plugins/claude/example_reviews/suggest_new_reviews.md`
 
-          These are the full, detailed versions of the two review instruction prompts. Read them to understand the review's intent, checklist, and tone. The inline YAML rules in steps 3 and 4 below are condensed versions — use the example files to fill in any gaps or to judge whether the inline version captures the key points.
-
-          For reference, this is how the DeepWork project's own `.deepreview` configures these two rules (using `instructions: { file: ... }` to point at the example files above):
-
-          ```yaml
-          prompt_best_practices:
-            description: "Review prompt/instruction markdown files for Anthropic prompt engineering best practices."
-            match:
-              include:
-                - "**/CLAUDE.md"
-                - "**/AGENTS.md"
-                - ".claude/**/*.md"
-                - ".deepwork/review/*.md"
-                - ".deepwork/jobs/**/*.md"
-            review:
-              strategy: individual
-              instructions:
-                file: .deepwork/review/prompt_best_practices.md
-
-          suggest_new_reviews:
-            description: "Analyze all changes and suggest new review rules that would catch issues going forward."
-            match:
-              include:
-                - "**/*"
-              exclude:
-                - ".github/**"
-            review:
-              strategy: matches_together
-              instructions:
-                file: .deepwork/review/suggest_new_reviews.md
-          ```
-
-          When creating rules for the target project, adapt the `match.include` patterns to its structure. The inline YAML in steps 3 and 4 uses inline `instructions:` text (suitable for projects that don't have the external instruction files), while the above uses `instructions: { file: ... }` references.
+          Read these for context on each review's intent, checklist, and tone. The inline YAML rules in steps 2 and 3 below are condensed versions suitable for emitting into the target project's `.deepreview` file — always emit the inline form (not `instructions: { file: ... }`), since target projects don't have the example files. Use the example files only as reference when deciding what the condensed inline text should say.
 
-          If the example files are not found (e.g., the plugin is installed differently), proceed using the inline YAML in steps 3 and 4 below.
+          ## Task
 
-          ### 2. Check the existing `.deepreview` file
+          ### 1. Check the existing `.deepreview` file
 
           Read the top-level `.deepreview` file if it exists. Check whether `suggest_new_reviews` and/or `prompt_best_practices` rules are already present.
 
           - If both rules already exist with reasonable configurations, skip to the output step — no changes needed.
           - If one or both are missing, proceed to add the missing rule(s).
           - If a rule exists but has a substantially different configuration (wrong strategy, missing match patterns), update it to match the specification below.
 
-          ### 3. Add the `prompt_best_practices` rule (if not present)
+          ### 2. Add the `prompt_best_practices` rule (if not present)
 
           Add to `.deepreview`:
 
@@ -144,7 +110,7 @@ workflows:
 
           Adapt the `match.include` patterns to the project if needed. Check for directories containing `.md` files that appear to be AI instruction files (e.g., `.gemini/`, `.cursorrules`, custom agent directories). If found, add those patterns too. You may add both missing rules in a single edit to the `.deepreview` file. The patterns above are the baseline.
 
-          ### 4. Add the `suggest_new_reviews` rule (if not present)
+          ### 3. Add the `suggest_new_reviews` rule (if not present)
 
           Add to `.deepreview`:
 
@@ -175,7 +141,7 @@ workflows:
                 5. If no rules are warranted, say so. An empty suggestion list is valid.
           ```
 
-          ### 5. Validate
+          ### 4. Validate
 
           Ensure the `.deepreview` file is valid YAML. Ensure both rules have all required fields: `description`, `match.include`, `review.strategy`, `review.instructions`.
 
@@ -267,7 +233,7 @@ workflows:
 
           ### migrated_skill_backups
 
-          All backup copies of deleted skills in `.deepwork/tmp/migrated_skills/`. If no skills were migrated, this output should contain a single file `.deepwork/tmp/migrated_skills/NONE.md` stating that no review-like skills were found.
+          All backup copies of deleted skills in `.deepwork/tmp/migrated_skills/`. If no skills were migrated, this output should be an empty list.
 
         inputs: {}
         outputs:
@@ -347,14 +313,12 @@ workflows:
           deepreview_files:
             required: true
             review:
-              strategy: individual
+              strategy: matches_together
               instructions: |
-                Read each .deepreview file and the documentation files its rules protect.
-                Verify that the trigger scope of each rule is as narrow as possible —
-                it should only fire when files that could actually affect the doc's accuracy
-                change. Consider whether having more separate reviews with narrower scope
-                is actually more efficient than a slightly wider, shared review — each
-                review spawns a sub-agent with material overhead.
+                Read all .deepreview files together along with the documentation files their
+                rules protect. The criteria below are cross-cutting — Documentation Covered
+                and Efficient Rule Count require a project-wide view of all rules at once,
+                which is why this review runs once over the full set rather than per file.
 
                 Evaluate the output against these criteria:
                 - Documentation Covered: Every project documentation file that describes the project itself has a corresponding rule (either newly created or pre-existing).
@@ -905,10 +869,8 @@ workflows:
               instructions: |
                 Read the dependency analysis from the previous step to verify the rule
                 faithfully implements the approved plan. Read the documentation file
-                referenced in the rule's instructions. Check the .deepreview file for
-                valid YAML syntax and consistency with existing rules.
+                referenced in the rule's instructions.
 
                 Evaluate the output against these criteria:
                 - Faithful Implementation: The rule accurately implements the dependency analysis from the previous step — same match patterns, same strategy, same rule name convention.
-                - Valid Configuration: The .deepreview YAML is syntactically valid and follows the schema. All required fields (description, match.include, review.strategy, review.instructions) are present.
                 - Effective Instructions: Review instructions clearly tell the reviewer to check whether the documentation file is still accurate given the source file changes. The documentation file path is explicitly referenced. Uses additional_context.unchanged_matching_files: true so the reviewer can read the doc even when only source files changed.

src/deepwork/standard_jobs/deepwork_reviews/job.yml

@@ -8,7 +8,7 @@ instructions: |
 
 
 matchers:
-  - ".deepwork/schemas/*/deepschema.yml"
+  - "**/deepschema.yml"
   - "**/.deepschema.*.yml"
 
 json_schema_path: "deepschema_schema.json"

src/deepwork/standard_schemas/deepschema/deepschema.yml

@@ -76,6 +76,21 @@ requirements:
     auto-pass rather than fail. Avoid criteria that penalize steps for not doing things
     outside their scope.
 
+  review-strategy-matches-criteria-scope: >
+    The review `strategy` SHOULD match whether the criteria need to reason across the
+    other changed files in the same review set. `strategy: individual` SHOULD be used
+    when the focus is on each file itself and the reviewer is unlikely to need to see
+    the other changed files in order to evaluate this one. A reviewer operating under
+    `individual` MAY consult unchanged project files for context (imports, references,
+    related code, conventions); such consultation does NOT require switching to
+    `matches_together`. `strategy: matches_together` SHOULD be used when the changed
+    files are likely to need to be looked at alongside each other during review.
+    `strategy: individual` MUST NOT be used when any criterion is a property of the
+    set of changed files as a whole (e.g., coverage across the set, deduplication of
+    the set, cross-file consistency, set-level minimality), because it forces each
+    spawned reviewer to redo the cross-file scan N times and multiplies latency
+    without improving review quality.
+
   review-no-redundant-context: >
     Review `instructions` MUST NOT repeat domain context already in `common_job_info` since
     the framework automatically prepends it to review prompts.
@@ -129,6 +144,20 @@ requirements:
     that are too large for inline strings. The existence of a `.deepwork/tmp/` path
     in a step_argument name is a strong signal the argument should be `type: string`.
 
+  no-sentinel-files: >
+    Steps SHOULD NOT use sentinel placeholder files (e.g., writing a `NONE.md` or
+    `EMPTY.md` file to satisfy a required `file_path` output when the real result is
+    "nothing to report") to work around the rule that `file_path` outputs cannot be
+    empty. If a step's output is a list that can legitimately be empty, the output
+    SHOULD be `type: string` (containing a narrative summary that naturally handles
+    the empty case) rather than `type: file_path` with a placeholder file. Sentinel
+    files leave stray artifacts on disk, confuse downstream steps that treat every
+    output path as a real file, and mislead reviewers. Exception: a sentinel file
+    MAY be used when the step genuinely needs a filesystem artifact a later step
+    will read and parse (e.g., a manifest with "no items" semantics baked into its
+    schema) — but the common "required output can't be empty" workaround is not a
+    sufficient reason.
+
   step-visibility-boundary: >
     A step's instructions MUST NOT reference information from steps that have not yet
     run in the same workflow, including steps from other workflows in the same job.