4.5 KiB
4.5 KiB
Code Review: fix-harness-command-template
Self-review against the SPEC and the harness-agnostic contract.
SPEC compliance
- R1
{prompt_content}substitution token: ✓ implemented in_invoke_harness(loop-runner.py). Reads the resolved prompt file's text; falls back to""on OSError. Single argv element undersubprocess.runlist mode. - R2 New default
["opencode", "run", "--dir", "{cwd}", "{prompt_content}"]: ✓ replaced both fallback branches (harness_cfg is None, and empty command array). - R3 No hardcoded
--modelin default: ✓ confirmed — model is inherited from opencode config. - R4 Per-role harness command override: ✓ not added (out of scope, per design).
- R5 Test stub updates: ✓
_make_loophelpers in 4 test files write loop-local prompt stubs only when the framework prompt at~/.automaton/prompts/<ref>does not already exist (preserves token-substitution tests in test_loop_templates). Custom-command tests now identify roles byimplement-prompt/verify-promptmatchers (the temp file path); default-command tests usetest-impletc. (matching the stub contentprompt: test-impl.md). - R6 Design doc update: ✓
design/loops/technical.md§8 and §9 (self-improvement template) updated to the new default; documented{prompt_content}alongside existing tokens; added Pi Dev, aider, and generic examples. - R7 Backwards compat: ✓
{prompt}and{cwd}tokens still populated; the existing custom-command test (TestHarnessSubstitution.test_custom_command_with_output_token) passes unchanged.
Harness-agnostic contract check
- Runner core has zero harness awareness: ✓ only token substitution, no
if harness == "opencode"branches. - D8 (no model/provider inspection): ✓ preserved; no model name appears in the runner core, only in user-overridable
harness.command. - The fix is MORE agnostic than v1: ✓ adds
{prompt_content}covering harnesses that prefer a message argument (aider, Pi Dev, any CLI taking a prompt as positional). v1 only supported file-path-based prompts.
Test plan compliance
Tests in tests/test_harness_command.py (7 new):
test_default_uses_dir_not_cwd— ✓ asserts--diris present,--cwdand--prompt-fileabsenttest_default_passes_prompt_content— ✓ asserts the prompt text appears as the last argv elementtest_prompt_content_handles_special_chars— ✓ asserts a prompt containing single quotes, double quotes, and dollar signs appears as a single argv elementtest_prompt_token_still_available— ✓ custom["cat", "{prompt}"]receives the temp file pathtest_custom_command_with_cwd_still_works— ✓ custom--cwdreceives the cwd valuetest_empty_command_falls_back_to_new_default— ✓ emptycommandarray falls back to--dir {cwd} {prompt_content}(NOT the old shape)test_pi_shaped_command_substitutes_correctly— ✓ proves the substitution mechanism works for a non-opencode binary (pi run --cwd {cwd} {prompt_content})
Existing tests updated (per SPEC R5):
_make_loophelpers in 4 test files now write loop-local prompt stubs with role-marker contentprompt: <ref>, preserving the substring-matcher strategy used by tick-flow tests. Skipped when the framework prompt exists (so test_loop_templates still substitutes real framework prompt tokens).- The
--prompt-filestub rule (fake_run.add_simple("--prompt-file", "")) is removed — the default matcher fallback handles generic invocations. - Custom
harness.commandtests using{prompt}token: matchers updated fromtest-impltoimplement-prompt(the resolved temp file path containstickN-implement-prompt.md). - Default-command tests using
{prompt_content}stub content: matchers staytest-impl(matches the stub contentprompt: test-impl.md).
Full suite: 440 passed (was 433; +7 new). No regressions.
Risks revisited
- Argv length: real prompts are 2-10KB; OS argv limit is 128KB+. Acceptable.
- Test mock drift: the
fake_runfixture now mocks a different default shape. A separate smoke test that shells out toopencode run --helpwould catch future flag renames. Not added in this task to keep the change focused; noted for a future hardening pass. The 7 new_invoke_harnessunit tests do cover the default-command construction directly, which is the main surface. - Pi Dev CLI: actual
pi runflags unverified (pi not installed on this machine). The Pi Dev test (test 7) uses a representative shape; the user confirms actual flags againstpi run --helpon their machine before going live.
Verdict
PASS — proceed to bug_find.