Skip to content

Bug: string-type output review blocks are silently not executed #350

@nhorton

Description

@nhorton

Summary

review: blocks declared on type: string step outputs in job.yml are silently not executed. The framework accepts the declaration, prepends context, and does everything up to the point of actually creating a ReviewRule — then drops the review on the floor because the review pipeline matches rules against file paths and string outputs have no file paths.

Evidence

In src/deepwork/jobs/mcp/quality_gate.py, inside build_dynamic_review_rules():

```python

Get file paths for file_path type

if arg.type == "file_path":
file_paths = [value] if isinstance(value, str) else list(value)
else:
# For string type, no file matching — create a synthetic task later
file_paths = []

for i, review_block in enumerate(review_blocks):
...
if arg.type == "file_path" and file_paths:
# Create exact-match patterns for the output files
...
rules.append(rule)
```

The comment at the `else:` branch says "create a synthetic task later" — but there is no "later". I grepped the whole codebase for `synthetic` and the only other hit is in the process-requirements review block further down in the same file, which is unrelated:

```
src/deepwork/jobs/mcp/quality_gate.py:184: # For string type, no file matching — create a synthetic task later
src/deepwork/jobs/mcp/quality_gate.py:266: # Create a synthetic ReviewTask directly (not a ReviewRule since there are
```

So the rule is only appended when `arg.type == "file_path" and file_paths`. String outputs fall through and nothing is ever added.

Impact

Any job.yml that declares a review on a string-typed output will have that review silently ignored. Authors get no warning, no error, and the review *looks* correctly configured at the schema level.

This also interacts with the `job_yml` DeepSchema's `no-temporary-files-for-transient-data` requirement, which directs authors to prefer `type: string` for transient inter-step data. Authors following that guidance and also declaring a `review:` block on the string output will not get the review they think they configured.

Expected behavior

Either:

  1. Implement the "synthetic task later" path: for `type: string` outputs with a review block, build a synthetic `ReviewTask` whose "content" comes from the string value itself (rather than matching against file paths). The reviewer agent would see the string value inline in its prompt and evaluate it against the review instructions.
  2. Fail validation at job load time if a `review:` block is declared on a `type: string` output, so the misconfiguration surfaces instead of silently passing.

Option 1 is the more useful outcome — string outputs are legitimate things to review (narrative summaries, counts, status reports).

Reproduction

  1. Define a job with a `type: string` output and a `review:` block on it
  2. Run the step via the MCP server and submit a value via `finished_step`
  3. Observe: quality gate returns clean, review instructions for that output are never emitted

Related

  • Discovered while working on fix: remove redundant review rules and fix strategy/criteria mismatch #348 — was considering changing `migrate_existing_skills.migrated_skill_backups` in `deepwork_reviews` from `file_path` to `string` until I verified this gap exists.
  • The `job_yml` DeepSchema at `src/deepwork/standard_schemas/job_yml/deepschema.yml` (requirements `type-selection`, `no-temporary-files-for-transient-data`) directs authors toward string type for transient data, making this gap more likely to be hit.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions