Implementation Plan: P2-WP3 Create research-review.yaml Sub-Recipe#1973
Conversation
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
| guard_pr_url: | ||
| action: route | ||
| on_result: | ||
| - when: "${{ context.pr_url }}" |
There was a problem hiding this comment.
[warning] defense: guard_pr_url routes the missing-pr_url fallback to review_pr_complete (the success terminal stop) rather than escalate_stop. If compose_research_pr captured an empty pr_url, the recipe silently terminates as if it succeeded. Consider routing the empty-pr_url branch to escalate_stop to surface the failure explicitly.
There was a problem hiding this comment.
Valid observation — flagged for design decision. The fallthrough to review_pr_complete is intentional graceful degradation (step note: 'Guard — skip review if pr_url is empty'). test_guard_pr_url_fallback_to_review_pr_complete asserts this routing. Whether to hard-fail on empty pr_url instead is a product-owner decision.
| step_name: review_research_pr | ||
| capture: | ||
| review_verdict: "${{ result.verdict }}" | ||
| skip_when_false: inputs.review_pr |
There was a problem hiding this comment.
[warning] defense: review_research_pr uses skip_when_false but has no explicit on_success — only on_result. Confirm that the recipe engine treats skip_when_false as advancing via on_result's unconditional route when the condition is false, otherwise a skipped step may leave the recipe in an undefined state.
There was a problem hiding this comment.
Valid observation — flagged for design decision. The step note documents 'All paths (skip, success, failure, context-limit) route to audit_claims'. Whether skip_when_false advances via on_result's unconditional route needs explicit verification from the recipe engine team.
| step_name: audit_claims | ||
| capture: | ||
| audit_verdict: "${{ result.verdict }}" | ||
| skip_when_false: inputs.audit_claims |
There was a problem hiding this comment.
[warning] defense: audit_claims uses skip_when_false but has no explicit on_success — only on_result/on_failure/on_exhausted/on_context_limit. Same skip_when_false advancement ambiguity as review_research_pr. Verify the engine handles the skipped-step advancement case explicitly.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Same skip_when_false advancement question as review_research_pr. Step note documents 'All paths (skip, success, failure) route to route_review_resolve'. Engine behavior verification needed from recipe engine team.
| stale_threshold: 2400 | ||
| idle_output_timeout: 0 | ||
| optional_context_refs: [worktree_path] | ||
| with: |
There was a problem hiding this comment.
[warning] defense: re_run_experiment on_context_limit routes to re_push_research, bypassing re_generate_report and re_stage_bundle. On context-limit, the report and bundle will be stale relative to the re-run results. Consider routing to re_generate_report instead.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Current on_context_limit → re_push_research is best-effort: push code fixes even if re-experiment didn't complete. Routing to re_generate_report risks hitting another context limit. Design decision for orchestration team.
| cmd: "cd '${{ context.worktree_path }}' && git push" | ||
| cwd: "${{ context.worktree_path }}" | ||
| step_name: re_push_research | ||
| on_success: finalize_bundle_render |
There was a problem hiding this comment.
[warning] cohesion: re_push_research routes on_failure to escalate_stop, diverging from the best-effort fallthrough pattern used by all other re-validation steps (re_run_experiment, re_generate_report, re_stage_bundle all route on_failure to the next step). Confirm whether push failure should hard-stop or fall through to finalize_bundle_render.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Push failure hard-stops while other re-validation steps fall through. Without a successful push the PR won't reflect re-validation results, so hard-stop may be correct. Design decision needed.
- Fix re_push_research on_success: routes to finalize_bundle instead of finalize_bundle_render to ensure context.report_path_after_finalize is populated before finalize_bundle_render runs - Remove duplicate LENS ITERATION note from run_experiment_lenses step (kitchen_rules already carries this directive) - Fix review_pr_complete stop message: use context.research_dir instead of inputs.research_dir (consistent with all other step references) - Document intentional ingredient shadowing in re_run_experiment and re_generate_report notes (experiment_results and report_path) - Fix test_prepare_research_pr_uses_context_worktree_path: use recipe fixture and step.with_args assertions instead of raw YAML string count - Update test_re_push_research_routes_to_finalize_bundle to match new routing - Remove narrating inline comments in test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review -- Verdict: changes_requested
| model: "" | ||
| optional_context_refs: [worktree_path, research_dir, report_path, experiment_plan, experiment_results, experiment_type, scope_report, visualization_plan_path] | ||
| with: | ||
| skill_command: "/autoskillit:prepare-research-pr ${{ context.report_path }} ${{ context.experiment_plan }} ${{ context.worktree_path }} ${{ inputs.base_branch }}" |
There was a problem hiding this comment.
[warning] defense: prepare_research_pr skill_command interpolates context.report_path and context.experiment_plan without shell quoting. Both are listed as optional_context_refs (so either may be absent) and both are path-like values that may contain spaces, causing argument splitting or silent empty-argument injection. Wrap in quotes: "${{ context.report_path }}" "${{ context.experiment_plan }}".
There was a problem hiding this comment.
Valid observation — flagged for design decision. prepare_research_pr skill_command interpolates context.report_path and context.experiment_plan as unquoted comma-separated path lists; shell word-splitting would affect paths containing spaces.
| capture: | ||
| html_path: "${{ result.html_path }}" | ||
| on_success: route_archive_or_export | ||
| on_failure: route_archive_or_export |
There was a problem hiding this comment.
[warning] arch: export_local_bundle calls autoskillit.recipe._cmd_rpc.export_local_bundle via run_python, accessing a private submodule by dotted path string. The prior review round noted this is the established pattern for run_python callables (verdict=REJECT). Raising for awareness only: if _cmd_rpc is intended as the stable recipe-layer RPC surface, promoting the callable to a public name would make that contract explicit and prevent silent breakage on rename.
There was a problem hiding this comment.
Valid observation — flagged for design decision. export_local_bundle accesses autoskillit.recipe._cmd_rpc (private submodule) via run_python. This pattern was previously investigated in comment 3192708589 (thread resolved as intentional: _cmd_rpc is the recipe-layer callable interface explicitly tested in tests/recipe/test_cmd_rpc.py).
| def test_requires_packs(self, recipe) -> None: | ||
| assert set(recipe.requires_packs) == {"research", "exp-lens", "vis-lens"} | ||
|
|
||
| def test_no_autoskillit_version(self) -> None: |
There was a problem hiding this comment.
[warning] tests: test_no_autoskillit_version bypasses the class fixture and reads the YAML file directly via path.read_text(). This continues the raw-file-access pattern resolved at L119. The assertion can be done on the loaded recipe model instead of raw text, keeping all structural assertions on the model.
There was a problem hiding this comment.
Valid observation — flagged for design decision. test_no_autoskillit_version reads the YAML file directly via path.read_text() rather than using the class-scoped recipe fixture. Flagged for consistent fixture usage review.
| content = path.read_text() | ||
| assert "patch_token_summary" not in content | ||
|
|
||
| def test_no_begin_archival_references(self) -> None: |
There was a problem hiding this comment.
[warning] tests: test_no_patch_token_summary_references (L176) and test_no_begin_archival_references (L181) both bypass the class fixture and do raw substring searches on the YAML file. The negative-guard intent is already fully covered by test_no_archival_steps (L162) which asserts against the parsed step name set. These raw-text tests are redundant with L162 and conflate YAML serialisation detail with semantic recipe state.
There was a problem hiding this comment.
Valid observation — flagged for design decision. test_no_patch_token_summary_references (L176) and test_no_begin_archival_references (L181) both bypass the class fixture with raw path.read_text() calls. Flagged for consistent fixture usage review.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: 1 blocking finding detected (critical/bugs). See inline comments for details — changes required before merge.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: 2 critical findings detected — changes required. (Self-authored PR: REQUEST_CHANGES downgraded to COMMENT.)
Blocking (critical):
tests/recipe/test_research_review_recipe.pyL133: Duplicate methodtest_finalize_bundle_failure_routes_to_escalate_stop— first definition assertson_success(wrong) and is silently shadowed.tests/recipe/test_research_review_recipe.pyL137: Second duplicate definition replaces first; remove the dead first definition.
⚠️ Outside Diff Range
tests/recipe/test_research_review_recipe.py
- L198 [warning/tests]: test_review_local_complete_sentinel_fields only asserts 'local_bundle_path' is present, while the parallel review_pr_complete test (L191-L196) checks four fields. If the local-complete sentinel carries additional context fields, they are untested.
- Fix re_push_research on_success: routes to finalize_bundle instead of finalize_bundle_render to ensure context.report_path_after_finalize is populated before finalize_bundle_render runs - Remove duplicate LENS ITERATION note from run_experiment_lenses step (kitchen_rules already carries this directive) - Fix review_pr_complete stop message: use context.research_dir instead of inputs.research_dir (consistent with all other step references) - Document intentional ingredient shadowing in re_run_experiment and re_generate_report notes (experiment_results and report_path) - Fix test_prepare_research_pr_uses_context_worktree_path: use recipe fixture and step.with_args assertions instead of raw YAML string count - Update test_re_push_research_routes_to_finalize_bundle to match new routing - Remove narrating inline comments in test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
force-pushed
the
p2-wp3-create-research-review-yaml-sub-recipe/1702
branch
from
c8d23c7 to
612f525
Compare
Add src/autoskillit/recipes/research-review.yaml — a standalone 22-step PR/review sub-recipe extracted from research.yaml. Receives campaign-injected hidden ingredients (worktree_path, research_dir, report_path, etc.), runs the review pipeline, and terminates with sentinel emission at review_pr_complete or review_local_complete. Key adaptations vs research.yaml: - Removes all archival steps (begin_archival, capture_experiment_branch, create_artifact_branch, open_artifact_pr, tag_experiment_branch, close_experiment_pr, patch_token_summary, research_complete) - Replaces archival routing with dual terminal stops: review_pr_complete (PR mode) and review_local_complete (local mode) - finalize_bundle.on_success → finalize_bundle_render (not re_push_research) - route_archive_or_export PR fall-through → review_pr_complete - export_local_bundle → review_local_complete on both success/failure - re_push_research → finalize_bundle_render on success - Uses optional_context_refs for campaign-injected context variables Also adds tests/recipe/test_research_review_recipe.py with 32 tests covering header, ingredients, steps, routing adaptations, negative guards, terminal stop sentinels, and kitchen_rules. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…view Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix re_push_research on_success: routes to finalize_bundle instead of finalize_bundle_render to ensure context.report_path_after_finalize is populated before finalize_bundle_render runs - Remove duplicate LENS ITERATION note from run_experiment_lenses step (kitchen_rules already carries this directive) - Fix review_pr_complete stop message: use context.research_dir instead of inputs.research_dir (consistent with all other step references) - Document intentional ingredient shadowing in re_run_experiment and re_generate_report notes (experiment_results and report_path) - Fix test_prepare_research_pr_uses_context_worktree_path: use recipe fixture and step.with_args assertions instead of raw YAML string count - Update test_re_push_research_routes_to_finalize_bundle to match new routing - Remove narrating inline comments in test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d_pr_url path review_pr_complete is reachable via guard_pr_url fallback when pr_url is empty, bypassing finalize_bundle. In that path, context.report_path_after_finalize is never captured and interpolates to empty string. Add a note to the stop message documenting this known-absent case so the LLM orchestrator emits empty string rather than silently misrepresenting the sentinel field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment decomposing 15 = 7 + 8 creates false precision since the filter does not distinguish the two groups; plain count is sufficient. Addresses reviewer comment 3192847593. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t ref - Remove LOCAL MODE OUTPUT kitchen_rule: file-layout documentation already in export_local_bundle step note (comment 3192848609) - Remove SENTINEL EMISSION kitchen_rule: restates terminal stop messages which already contain the sentinel field list (comment 3192848715) - Remove redundant cd in re_push_research cmd: cwd field already sets working directory; sibling steps use cwd-only (comment 3192848495) - Add all_diagram_paths to finalize_bundle_render optional_context_refs: capture_list output may be absent if no lenses ran (comment 3192848381) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ruff E501 violations introduced by develop commit 2577763 — two docstrings exceeded the 99-char line limit after rebasing onto upstream/develop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
force-pushed
the
p2-wp3-create-research-review-yaml-sub-recipe/1702
branch
from
c437b07 to
9b52626
Compare
…1973) ## Summary Create `src/autoskillit/recipes/research-review.yaml` as a standalone sub-recipe containing the 22-step PR/review phase extracted from `research.yaml`. The recipe receives campaign-injected hidden ingredients (`worktree_path`, `research_dir`, `report_path`, `experiment_plan`, `experiment_results`, `experiment_type`, `scope_report`, `visualization_plan_path`), lifts all review steps verbatim-and-adapted, replaces the archival phase with dual terminal stops (`review_pr_complete` for PR mode, `review_local_complete` for local mode), and corrects routing targets to terminate within this sub-recipe rather than routing to archival or non-existent steps. Closes #1702 ## Implementation Plan Plan file: `/home/talon/projects/autoskillit-runs/impl-20260505-184855-274993/.autoskillit/temp/make-plan/p2_wp3_create_research_review_yaml_plan_2026-05-05_185500.md` 🤖 Generated with [Claude Code](https://claude.com/claude-code) via AutoSkillit <!-- autoskillit:pipeline-signature steps=prepare_pr,run_arch_lenses,compose_pr,annotate_pr_diff,review_pr --> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
Create
src/autoskillit/recipes/research-review.yamlas a standalone sub-recipe containing the 22-step PR/review phase extracted fromresearch.yaml. The recipe receives campaign-injected hidden ingredients (worktree_path,research_dir,report_path,experiment_plan,experiment_results,experiment_type,scope_report,visualization_plan_path), lifts all review steps verbatim-and-adapted, replaces the archival phase with dual terminal stops (review_pr_completefor PR mode,review_local_completefor local mode), and corrects routing targets to terminate within this sub-recipe rather than routing to archival or non-existent steps.Closes #1702
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260505-184855-274993/.autoskillit/temp/make-plan/p2_wp3_create_research_review_yaml_plan_2026-05-05_185500.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary
Token Efficiency