Rectify: research recipe archives wrong experiment directory; replace find heuristic with context tracking (#655)#658
Conversation
Adds test_rules_cmd.py with 7 tests for the new run-cmd-emit-alignment and run-cmd-find-rediscovery semantic rules, and extends test_research_artifact_archive.py with 7 tests covering the research_dir echo/capture contract, find-heuristic removal, post-compression guard, and scoped checkout in create_artifact_branch. All tests fail until rules_cmd.py and research.yaml are fixed.
…rchive path
Adds rules_cmd.py with two semantic rules:
- run-cmd-emit-alignment (ERROR): every non-raw capture key in a run_cmd step
must have a matching echo "KEY=..." in the cmd; catches silent empty captures
- run-cmd-find-rediscovery (WARNING): flags find|sort|tail heuristics that
signal an upstream step failed to capture a path into context
Fixes research.yaml:
- create_worktree now emits and captures research_dir so the exact directory
path flows through context without re-discovery
- commit_research_artifacts uses ${{ context.research_dir }} directly instead
of find|sort|tail -1 (which picked the wrong dir when multiple existed)
- Adds post-compression guard: fails loudly if artifacts.tar.gz is absent,
uncompressed dir still present, or README.md is missing
- create_artifact_branch scopes the checkout to the specific experiment subdir
via basename, preventing blanket research/ copy
- open_artifact_pr detects at runtime whether tarball or raw artifacts/ exists
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested (COMMENT mode: cannot request changes on own PR)
| if not isinstance(cmd, str): | ||
| continue | ||
| for cap_key, cap_val in step.capture.items(): | ||
| m = _RESULT_CAPTURE_RE.search(cap_val) |
There was a problem hiding this comment.
[warning] defense: If step.capture is None (a step with no capture block), calling step.capture.items() will raise AttributeError. Add a None-guard: if not step.capture: continue before the loop.
There was a problem hiding this comment.
Investigated — this is intentional. In schema.py, RecipeStep.capture is declared as capture: dict[str, str] = field(default_factory=dict). The default_factory=dict guarantees the field is always an empty dict, never None. Calling step.capture.items() cannot raise AttributeError.
| result_key = m.group(1) | ||
| if result_key in _RAW_RESULT_FIELDS: | ||
| continue | ||
| # Check that the cmd emits `echo "result_key=..."`. |
There was a problem hiding this comment.
[warning] slop: Inline comment # Check that the cmd emits \echo "result_key=..."`.` restates exactly what the next two lines do without adding any reasoning about why. Remove it.
There was a problem hiding this comment.
Flagged for design decision. This inline comment was classified below the warning threshold (cosmetic/slop). Deferring to human review — no automatic change applied.
| if result_key in _RAW_RESULT_FIELDS: | ||
| continue | ||
| # Check that the cmd emits `echo "result_key=..."`. | ||
| echo_pattern = re.compile(rf'\becho\s+"?{re.escape(result_key)}=') |
There was a problem hiding this comment.
[warning] bugs: The echo_pattern r'\becho\s+"?{result_key}=' allows an optional double-quote but not a single-quote. A cmd using echo 'KEY=value' (single-quoted) will not match, producing a false-positive alignment error. Update pattern to r'\becho\s+[\"\\']?{result_key}=' to accept both quote styles.
There was a problem hiding this comment.
Investigated — this is intentional. The rule enforces a documented double-quoted echo convention. The error message prescribes echo "result_key=${...}" as the required form, and the rule description states echo "K=...". This validator intentionally rejects single-quoted cmds that deviate from the documented contract; widening the pattern would silently pass non-conforming recipes.
| from __future__ import annotations | ||
|
|
||
| from autoskillit.recipe.validator import run_semantic_rules | ||
| from tests.recipe.conftest import _make_workflow |
There was a problem hiding this comment.
[warning] tests: Importing the private helper _make_workflow directly from tests.recipe.conftest creates brittle coupling — any conftest refactor silently breaks this import. Expose it as a fixture or move it to a shared test utility module.
There was a problem hiding this comment.
Investigated — this is intentional. Direct import of _make_workflow from tests.recipe.conftest is the established pattern across the entire tests/recipe/ directory: test_rules_worktree.py, test_validator.py, test_rules_dataflow.py, and test_rules_structure.py all use the identical import. The underscore prefix signals it is not a pytest fixture, not that it is private. The comment misreads a deliberate, widely-replicated convention as brittleness.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review found 8 blocking issues. See inline comments. (verdict: changes_requested — unable to formally request changes on own PR)
…s-line false positives
…tree_path in open_artifact_pr
…ntext.research_dir
…nsitivity
tests/infra/test_claude_md_critical_rules.py and test_docstring_labels.py
used Path(__file__).parent.parent.parent (and bare relative paths like
Path("CLAUDE.md")) which depend on CWD being the project root at fixture
execution time. In CI with xdist, monkeypatch.chdir(tmp_path) calls in
neighboring infra tests (test_quota_check.py) run on the same worker and
can leave CWD temporarily changed; if __file__ is resolved as a relative
path by the pytest import machinery, REPO_ROOT becomes Path(".") and file
reads pick up the wrong location.
Fix: use Path(__file__).resolve() so REPO_ROOT is always an absolute path
regardless of CWD or import mode.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
deleted the
research-recipe-archives-wrong-experiment-directory-replace/655
branch
1 participant
Closes #655
Summary
Replaces the fragile
find | sort | tail -1heuristic inresearch.yamlwith deterministiccontext.research_dirtracking. Adds two new semantic rules (run-cmd-emit-alignment,run-cmd-find-rediscovery) to catch this class of bug at validation time.Changes
research.yaml:create_worktreeemitsresearch_dirinto context;commit_research_artifactsuses it directly;create_artifact_branchcheckout scoped to specific experiment dir; post-compression guard added;open_artifact_prbody made dynamicsrc/autoskillit/recipe/rules_cmd.py: two new semantic rules registered in__init__.pytests/recipe/test_rules_cmd.py: 7 new tests for the semantic rulestests/recipe/test_research_artifact_archive.py: 7 new tests for context tracking and guard behavior