Skip to content

feat(spec-loop): add harness-neutral SPEC_LOOP_EFFORT setting#958

Open
Shaurya2k06 wants to merge 3 commits into
apache:mainfrom
Shaurya2k06:feat/spec-loop-effort
Open

feat(spec-loop): add harness-neutral SPEC_LOOP_EFFORT setting#958
Shaurya2k06 wants to merge 3 commits into
apache:mainfrom
Shaurya2k06:feat/spec-loop-effort

Conversation

@Shaurya2k06

Copy link
Copy Markdown

Summary

  • Add SPEC_LOOP_EFFORT=low|medium|high so the spec loop can set per-invocation
    reasoning effort without binding to one vendor's flag vocabulary
    (PRINCIPLES.md §9).
  • Map the framework band onto each harness CLI's own knob (--effort,
    -c model_reasoning_effort=…, --variant); Cursor/Gemini omit silently.
    Framework high maps to each CLI's top value (max / xhigh).
  • Unset leaves argv unchanged; any other value fails at startup before the
    first iteration.

Assisted by Cursor Grok 4.5

Type of change

  • Tool / bridge contract (tools/<system>/*.md)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • CI / dev loop (prek, workflows, validators)
  • Other: uv.lock metadata refresh triggered by the vendor-neutrality
    hook when tools/spec-loop/README.md changed (mypy specifier sync)

Test plan

  • bash tools/spec-loop/tests/test_runner_fixtures.sh passes
    (set-and-mapped, unsupported harness, unset)
  • uv run --project tools/spec-validator spec-validate tools/spec-loop/specs/
  • prek run --files on touched paths passes
  • Invalid SPEC_LOOP_EFFORT=nonsense exits 1 with accepted-values message
  • SPEC_LOOP_EFFORT=high prints Effort: high in the startup banner

RFC-AI-0004 compliance

  • Vendor neutrality — framework vocabulary is low|medium|high;
    each harness maps to its own flag (no Claude-only binding)

Linked issues

Closes #925

Notes for reviewers

  • Mapping table lives in tools/spec-loop/specs/spec-loop-runner.md
    (follow-on effort table under the harness contract).
  • Kiro gets --effort when set (CLI has that flag) but still never gets
    --model (unchanged).

Map low/medium/high onto each agent CLI's own effort flag so mechanical
and hard passes can run at different reasoning levels without binding
the framework to one vendor's vocabulary.

Generated-by: Cursor Grok 4.5
Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
@justinmclean

Copy link
Copy Markdown
Member

Thanks for the contribution - it mostly good but needs a couple of improvements, I think:

What is good

  • Follows the existing $output_format translation pattern inside spec_loop_launch_agent() exactly, including the empty-array-safe ${arr[@]+"${arr[@]}"} idiom.
  • Spec, README and code land together, which satisfies the spec-sync pre-check in AGENTS.md § Commit and PR conventions.
  • Validation fails at startup rather than mid-iteration, and the banner prints the resolved band.
  • No Co-Authored-By: trailer; PR template filled in; issue linked.

Findings

Major — mapped high is the only band the tests exercise (tools/spec-loop/tests/test_runner_fixtures.sh)

Every new set-and-mapped fixture passes high. low and medium are untested for all four supported harnesses. That matters most for OpenCode: low maps to minimal, the single non-identity low mapping in the whole table, and it has zero coverage. A typo there would ship silently.

Please add at least an OpenCode low case asserting <--variant> / <minimal>, and one medium case.

Major — no set-but-unsupported fixture for Gemini (tools/spec-loop/tests/test_runner_fixtures.sh:~152)

The spec table lists both Cursor and Gemini CLI as no-knob harnesses, and issue #925's acceptance criteria says "a harness with no such knob adds nothing and prints no warning". Only Cursor is covered. Please mirror the Cursor block for gemini so the --yolo branch is pinned too.

Major — the per-harness flag names are not attributed to a source (tools/spec-loop/specs/spec-loop-runner.md:114)

Issue #925 asks for the flag name to be taken "from that CLI's own --help". --effort max (Claude Code), model_reasoning_effort=xhigh (Codex), --variant minimal|medium|max (OpenCode) and --effort (Kiro) are all version-dependent surfaces. Unlike the omit-silently path, a value the installed CLI does not recognise is a hard launch failure part-way through a loop run.

Minor — the "adding a harness" contract was not updated (docs/adapters/add-a-harness.md:162)

Step 3 is the documented checklist for adding a harness, and its lib.sh template still lists only the unattended flag, --model and the prompt transport. A contributor following it now produces a harness that silently has no effort support and never learns there was a decision to make. Please add the effort case to that template.

Related, one line below the new table in specs/spec-loop-runner.md:118: "Adding a new harness means extending this matrix, documenting the safety boundary, and updating loop.sh in the same change." There are now two matrices, and the change lands in lib.sh, not loop.sh. Worth rewording in the same pass.

Minor — startup validation has no automated coverage (tools/spec-loop/loop.sh:122)

The invalid-value exit is listed in the test plan as a manual check. The fixtures only reach lib.sh. Not a blocker given the fixture harness never starts loop.sh, but worth a note in the test file so the gap is deliberate rather than accidental.

Minor — README summary omits the one asymmetric mapping (tools/spec-loop/README.md:142)

The README says framework high maps to each CLI's top value, which is accurate, but a reader could reasonably assume low and medium pass through unchanged. OpenCode's low is minimal. One clause pointing at the spec table for the exact low/medium values would close it.

This review was drafted by an AI-assisted tool and confirmed by a Magpie maintainer. After you've addressed the points above and pushed an update, a Magpie maintainer, a real person, will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.

Shaurya2k06 and others added 2 commits July 27, 2026 13:15
Cover OpenCode low→minimal and a medium mapping, pin Gemini's silent
omit path, attribute effort flags to CLI --help sources, and update the
add-a-harness checklist so new harnesses decide effort wiring explicitly.

Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>

@justinmclean justinmclean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This covers everything from the last pass. The assert_not_contains "" on the OpenCode case is better than what I asked for: it pins that the framework band name never leaks into argv, which is the failure that mapping would actually produce. Good call recording the untestable loop.sh startup gate as a comment rather than dropping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a harness-neutral reasoning-effort setting to the spec loop

2 participants