diff --git a/.opencode/agents/task-implementor.md b/.opencode/agents/task-implementor.md index 5437ddb95..6483913ce 100644 --- a/.opencode/agents/task-implementor.md +++ b/.opencode/agents/task-implementor.md @@ -198,15 +198,33 @@ See the `implementer-helpers` skill (`SKILL.md`) for full subcommand documentati ### Main task -This is where actual implementation happens. Choose the appropriate procedure based on `work_type` from the subsections below: +This is where actual implementation happens. Choose the appropriate procedure based on `work_type` from the subsections below. + +**Pre-fetched context (read the prompt FIRST, before any GET / webfetch / git-isolator-util call).** As of 2026-05-10 the dispatcher defaults to populating the prompt with pre-fetched Forgejo data and a pre-cloned working copy. Look for these section headings in your prompt: + +| Section heading | What it carries | If present, skip this step | +|----------------------------------------------|-------------------------------------------------------|------------------------------------------------------------------| +| `## Pre-fetched PR description` | Title + full body of the PR (`pr_fix`, `request_changes_pr`) | The opening "Read the PR" GET below | +| `## Pre-fetched issue body` | Full body of the linked issue (`issue_impl`) | The opening "Read the issue" GET below | +| `## Pre-fetched diff` (between `BEGIN_PR_DIFF` / `END_PR_DIFF`) | Unified diff of changed files | Any "discover what changed" `read` / `grep` you'd otherwise do | +| `## Pre-fetched CI status` | Latest CI status + per-check detail | The opening "Fetch CI failure details" GET | +| `## Pre-fetched PR comments` / `## Pre-fetched issue comments` | Paginated comment history | The opening "Read all PR comments" GET | +| `## Pre-fetched active REQUEST_CHANGES reviews` | Active review-state-change feedback (`request_changes_pr`) | The opening "Read all reviews" GET | +| `## Pre-fetched linked issues` | Body + metadata of issues this PR closes / refs | A GET on each `Closes #N` target | +| `## Pre-fetched Epic` | Body of the parent Epic (when `Epic: #N` in body) | A GET on the Epic | +| `## Pre-cloned working copy` | `repo_dir` of a working clone the dispatcher set up | The `git-isolator-util` call in step 3 / 5 below | + +If a section heading appears but its body says **"The dispatcher attempted to pre-fetch this section but it was not available"**, the fetch failed (transient API error, network blip) — fall through to the original GET / webfetch / `git-isolator-util` step. The dispatcher's `## Data completeness` block (when present) lists the specific failures so you know which sections to backfill. If a section heading is **entirely absent** from your prompt, an operator has explicitly opted out via `IMPLEMENTER_DISPATCHER_PREFETCH=0` / `IMPLEMENTER_DISPATCHER_PRECLONE=0` (typically for a bisect or rollback) — also fall through to the original GET-based step. + +This is a **performance** change, not a correctness change: the pre-fetched data is functionally identical to what you'd `curl` yourself. Reading the prompt costs zero tool calls. Re-fetching data already in the prompt burns ~10-15 s per redundant GET and was the dominant cost in the pre-2026-05-10 implementer post-mortems. #### Procedure: `issue_impl` (New Issue Implementation) -1. **Read the issue.** GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}` — read title, body, labels, milestone, and metadata section (branch name, commit message format). Paginate all comments to understand full context and any subtask structure. +1. **Read the issue.** **If `## Pre-fetched issue body` is in your prompt, use it verbatim — skip the GET.** Otherwise, GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}` — read title, body, labels, milestone, and metadata section (branch name, commit message format). Paginate all comments to understand full context and any subtask structure (or use `## Pre-fetched issue comments` when present). 2. **Determine branch name.** Extract the branch name from the issue's Metadata section if present. If absent, derive one: `feature/issue-{work_number}-{kebab-slug-of-title}`. -3. **Create isolated clone.** Call `git-isolator-util` with `create_branch: true`, `base_branch: master`, and the determined `branch_name` (see Subagents section for prompt template). +3. **Create isolated clone.** **If `## Pre-cloned working copy` is present with a non-empty `repo_dir`, use that path verbatim — the dispatcher has already cloned and checked out the branch for you. Skip the `git-isolator-util` call entirely.** Otherwise, call `git-isolator-util` with `create_branch: true`, `base_branch: master`, and the determined `branch_name` (see Subagents section for prompt template). 4. **Implement the code.** Load the `cleverthis-guidelines` skill for CONTRIBUTING.md rules and follow them strictly. Key rules: - Source in `src/cleveragents/`, Behave unit tests in `features/`, Robot Framework integration/e2e tests in `robot/` @@ -248,15 +266,15 @@ This is where actual implementation happens. Choose the appropriate procedure ba #### Procedure: `pr_fix` (PR Fix) -1. **Read the PR.** GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/pulls/{work_number}` — read description, head branch, head SHA, and CI state. Set `branch_name` to the PR's head branch. +1. **Read the PR.** **If `## Pre-fetched PR description` is in your prompt, use it verbatim — read description, head branch, head SHA, and CI state from there. Skip the GET.** Otherwise, GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/pulls/{work_number}` — read description, head branch, head SHA, and CI state. Set `branch_name` to the PR's head branch (or to the `head_ref` in the pre-fetched section). -2. **Read all reviews.** GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/pulls/{work_number}/reviews?limit=50&page=N` — paginate fully. For any review in `REQUEST_CHANGES` state, GET its comments to understand the specific feedback. +2. **Read all reviews.** **If `## Pre-fetched active REQUEST_CHANGES reviews` is in your prompt, use it verbatim — it already includes per-review comments. Skip the GET.** Otherwise, GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/pulls/{work_number}/reviews?limit=50&page=N` — paginate fully. For any review in `REQUEST_CHANGES` state, GET its comments to understand the specific feedback. -3. **Read all PR comments.** GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}/comments?limit=50&page=N` — paginate fully. +3. **Read all PR comments.** **If `## Pre-fetched PR comments` is in your prompt, use it verbatim. Skip the GET.** Otherwise, GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}/comments?limit=50&page=N` — paginate fully. -4. **Fetch CI failure details.** GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/commits/{head_sha}/statuses?limit=50&page=N` — paginate fully. For each failing status that has a `target_url`, webfetch that URL to retrieve the failure logs. +4. **Fetch CI failure details.** **If `## Pre-fetched CI status` and `## Pre-fetched CI per-check detail` are in your prompt, use them verbatim — the per-check section already carries failure URLs and (when the dispatcher could reach them) the fetched failure-log excerpts. Skip both the GET and the per-status `webfetch`.** Otherwise, GET `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/commits/{head_sha}/statuses?limit=50&page=N` — paginate fully. For each failing status that has a `target_url`, webfetch that URL to retrieve the failure logs. -5. **Create isolated clone.** Call `git-isolator-util` with `create_branch: false` and `branch: {branch_name}` (the PR's head branch, resolved in step 1). +5. **Create isolated clone.** **If `## Pre-cloned working copy` is present with a non-empty `repo_dir`, use that path verbatim — the dispatcher has already cloned the PR's head branch and checked it out for you. Skip the `git-isolator-util` call entirely; proceed to step 6 with `repo_dir` taken from the pre-cloned section.** Otherwise, call `git-isolator-util` with `create_branch: false` and `branch: {branch_name}` (the PR's head branch, resolved in step 1). 6. **Fix the issues.** Address all CI failures and all unresolved reviewer feedback. Never partially address reviewer comments — every `REQUEST_CHANGES` concern must be fully resolved. diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fc5363be..41804c99a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,87 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed +- **Default-flip `IMPLEMENTER_DISPATCHER_PREFETCH` and + `IMPLEMENTER_DISPATCHER_PRECLONE` to ON (2026-05-10, Phase 4 + acceptance).** Both env-var flags now default to ON when unset + (matching the auto-agents-tier-2-3-plan § 5b acceptance gate). + Operators who need the legacy title-only prompt / in-session + `git-isolator-util` clone for a bisect or rollback set the + corresponding env var to `0` explicitly. Behaviour summary: + + | env var value | `_is_prefetch_enabled()` | Notes | + |---|---|---| + | unset | True | new default (post-flip) | + | empty string `""` | True | falls through to default | + | `0` / `false` / `no` / `off` | False | explicit opt-out | + | `1` / `true` / `yes` / `on` | True | explicit opt-in | + + Same matrix applies to `IMPLEMENTER_DISPATCHER_PRECLONE`. The + ``_env_truthy`` helper was renamed in spirit (kept as-is for + the truthy-side path) and joined by a new + ``_env_falsy_explicit`` helper that only treats the four + falsy literals as opt-outs — empty string and unset both + fall through to the new default. + + Why this matters: the post-mortem of the PR #30 implementer + runs showed that without prefetch the ``task-implementor`` + subagent spent the first ~3-4 minutes on curl + grep + read + calls just to discover head SHA, base branch, and changed + files — all of which the dispatcher already has from its own + Forgejo calls. The pre-clone elimination saves another + ~60-100 s for ``pr_fix`` work by skipping the + ``git-isolator-util`` subagent's clone-then-checkout dance. + Combined estimated saving from one cycle: ~260-400 s. + + Concretely: + + - ``tools/dispatch_implementer.py``: ``_is_prefetch_enabled`` + and ``_is_preclone_enabled`` rewritten to invert the + default. Each grew a docstring with the full behaviour + matrix. + - ``tests/auto_agents/test_implementer_prompt_snapshot.py``: + the autouse fixture switched from ``delenv`` to + ``setenv("...", "0")`` so legacy-snapshot tests pin to the + opt-out path explicitly. Tests that exercise the rich + prompt continue to override with ``setenv("...", "1")``. + - ``tests/auto_agents/test_dispatch_implementer.py``: + ``test_legacy_prompt_when_flag_unset`` was reworked into + two regression guards + (``test_prefetch_enabled_by_default_when_flag_unset`` + + ``test_preclone_enabled_by_default_when_flag_unset``) that + pin the new default. The + ``test_falsy_values_disable_prefetch`` parametrize dropped + `""` (empty string is no longer a falsy opt-out). A new + ``test_empty_string_falls_through_to_default`` test pins + the empty-string-equals-unset contract for both flags. + ``test_clone_section_no_handle_when_flag_disabled`` now + sets ``IMPLEMENTER_DISPATCHER_PRECLONE=0`` explicitly + instead of relying on the unset-default that no longer + means "off". + +- **task-implementor.md: read the prompt FIRST before any + Forgejo GET / webfetch / `git-isolator-util` call (2026-05-10).** + The MD now includes a "Pre-fetched context" table at the top + of the Main task section listing every prompt section the + dispatcher may populate (PR description, diff, CI status, + comments, reviews, linked issues, Epic, pre-cloned working + copy) and which procedure step that section short-circuits. + Each numbered step in the ``pr_fix`` and ``issue_impl`` + procedures now leads with **"If `## Pre-fetched ...` is in + your prompt, use it verbatim — skip the GET"**. The fallback + to the original GET-based step is preserved for the explicit + opt-out path (``IMPLEMENTER_DISPATCHER_PREFETCH=0``) and for + the transient-failure case where the dispatcher attempted + the fetch but couldn't reach Forgejo (the section renders an + ``"unavailable"`` placeholder). + + Why this matters: without this MD update the prefetched + data would land in the prompt but the worker would still + ``curl`` for it anyway — defeating the purpose of the + default-flip above. The two changes are complementary; the + ``task-implementor.md`` change is what makes the + ``dispatch_implementer.py`` change actually save wallclock. + - **Tighten `implementation-worker.md` skill allowlist and Step 0 (2026-05-10).** Post-P0 rerun against PR #30 showed the wrapper still loading the heavier `auto-agents-system` skill (~29 KB, diff --git a/tests/auto_agents/test_dispatch_implementer.py b/tests/auto_agents/test_dispatch_implementer.py index e63a82fad..bf43e2392 100644 --- a/tests/auto_agents/test_dispatch_implementer.py +++ b/tests/auto_agents/test_dispatch_implementer.py @@ -141,10 +141,35 @@ def _issue_item(number: int = 42, title: str = "Add JWT refresh") -> dict[str, A class TestPrefetchEnvFlag: - def test_legacy_prompt_when_flag_unset( - self, driver, cfg, monkeypatch + def test_prefetch_enabled_by_default_when_flag_unset( + self, driver, monkeypatch ): + """Post-2026-05-10: the default is **ON**. An unset env var + falls through to the dispatcher's documented default (rich + prefetched prompt). This is the regression guard for the + default-flip itself — if a future refactor accidentally + re-introduces the off-by-default behaviour, this test fails + loudly. Tests that need the legacy prompt set the env var to + ``"0"`` explicitly (see ``test_legacy_prompt_when_flag_falsy`` + below).""" monkeypatch.delenv("IMPLEMENTER_DISPATCHER_PREFETCH", raising=False) + assert driver._is_prefetch_enabled() is True + + def test_preclone_enabled_by_default_when_flag_unset( + self, driver, monkeypatch + ): + """Symmetric to the prefetch default-flip: pre-clone is also + ON when the env var is unset post-2026-05-10.""" + monkeypatch.delenv("IMPLEMENTER_DISPATCHER_PRECLONE", raising=False) + assert driver._is_preclone_enabled() is True + + def test_legacy_prompt_when_flag_falsy(self, driver, cfg, monkeypatch): + """Explicit ``IMPLEMENTER_DISPATCHER_PREFETCH=0`` (the + opt-out path for a bisect / rollback / dry-run) restores + the legacy title-only prompt. The legacy code path must + continue to work for the foreseeable future — this is the + rollback safety net.""" + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "0") item = _pr_item() group = driver.WORK_GROUPS[0] prompt = driver._implementation_prompt_dispatch(cfg, item, group) @@ -152,13 +177,6 @@ class TestPrefetchEnvFlag: assert "PR Compliance Checklist" in prompt assert "_dispatcher_implementer_context" not in item - def test_legacy_prompt_when_flag_falsy(self, driver, cfg, monkeypatch): - monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "0") - item = _pr_item() - group = driver.WORK_GROUPS[0] - prompt = driver._implementation_prompt_dispatch(cfg, item, group) - assert "Pre-fetched" not in prompt - def test_prefetch_prompt_when_flag_truthy( self, driver, cfg, monkeypatch, fake_implementer_api ): @@ -183,14 +201,36 @@ class TestPrefetchEnvFlag: @pytest.mark.parametrize("value", ["1", "true", "TRUE", "yes", "on"]) def test_truthy_values_enable_prefetch(self, driver, monkeypatch, value): + """Truthy literals still flip prefetch ON. Backwards-compatible + with the pre-flip ``_env_truthy`` contract — operators who set + the env var explicitly continue to get the expected behaviour + regardless of whether the default changed.""" monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", value) assert driver._is_prefetch_enabled() is True - @pytest.mark.parametrize("value", ["0", "false", "no", "off", ""]) + @pytest.mark.parametrize("value", ["0", "false", "no", "off"]) def test_falsy_values_disable_prefetch(self, driver, monkeypatch, value): + """Only the **explicit** falsy literals opt out of prefetch. + Empty string / unset fall through to the default-ON path — see + ``test_prefetch_enabled_by_default_when_flag_unset`` above and + ``test_empty_string_falls_through_to_default`` below for the + positive-control tests.""" monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", value) assert driver._is_prefetch_enabled() is False + def test_empty_string_falls_through_to_default(self, driver, monkeypatch): + """An empty-string env var (e.g. ``IMPLEMENTER_DISPATCHER_PREFETCH=`` + in a ``.env`` file) is NOT a falsy opt-out — it falls through + to the default-ON path. This matches Python's ``os.environ`` + convention where ``""`` and unset are routinely conflated, and + avoids surprising an operator who exports an empty-string + value (the typical mistake is ``export FOO=`` rather than + ``unset FOO``).""" + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "") + assert driver._is_prefetch_enabled() is True + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PRECLONE", "") + assert driver._is_preclone_enabled() is True + class TestPrFixPrefetch: def test_pr_fix_prompt_contains_all_sections( @@ -449,8 +489,13 @@ class TestPreclone: def test_clone_section_no_handle_when_flag_disabled( self, driver, cfg, monkeypatch, fake_implementer_api ): + # Post-2026-05-10 the preclone default is ON; "disabled" here + # means the explicit ``=0`` opt-out path rather than the + # historical "unset → off" assumption. Without this explicit + # set, the test would silently invoke the real + # ``prepare_pr_worktree`` and break in CI. monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "1") - monkeypatch.delenv("IMPLEMENTER_DISPATCHER_PRECLONE", raising=False) + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PRECLONE", "0") stub_forgejo_pr_details(fake_implementer_api) stub_forgejo_ci_status(fake_implementer_api, state="success") stub_forgejo_pr_comments(fake_implementer_api) diff --git a/tests/auto_agents/test_implementer_prompt_snapshot.py b/tests/auto_agents/test_implementer_prompt_snapshot.py index fa1b46f29..dbf05aa52 100644 --- a/tests/auto_agents/test_implementer_prompt_snapshot.py +++ b/tests/auto_agents/test_implementer_prompt_snapshot.py @@ -73,11 +73,19 @@ def cfg(tmp_path): @pytest.fixture(autouse=True) def _disable_flags(monkeypatch): """Each test in this module asserts on the legacy / prefetch / - preclone branch its scenario describes; reset the flags between - tests so an unset env var doesn't leak prefetch state from a - sibling test.""" - monkeypatch.delenv("IMPLEMENTER_DISPATCHER_PREFETCH", raising=False) - monkeypatch.delenv("IMPLEMENTER_DISPATCHER_PRECLONE", raising=False) + preclone branch its scenario describes; pin the flags to ``0`` + (explicit opt-out) by default so legacy-snapshot tests don't + silently flip when the dispatcher's default behaviour changes. + + Pre-flip (before 2026-05-10) this fixture used ``delenv`` because + "unset" meant "legacy off". Post-flip the default is ON, so we + have to be explicit: tests that want the rich prompt override + with ``setenv("...", "1")``, tests that want legacy inherit the + ``"0"`` set here, and the dispatcher's default-flip is verified + by a dedicated test in ``test_dispatch_implementer.py``. + """ + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "0") + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PRECLONE", "0") yield diff --git a/tools/dispatch_implementer.py b/tools/dispatch_implementer.py index 586ec533d..841d28c50 100644 --- a/tools/dispatch_implementer.py +++ b/tools/dispatch_implementer.py @@ -77,12 +77,60 @@ def _env_truthy(name: str) -> bool: return os.environ.get(name, "").strip().lower() in _TRUTHY_ENV_VALUES +_FALSY_ENV_VALUES = frozenset({"0", "false", "no", "off"}) + + +def _env_falsy_explicit(name: str) -> bool: + """``True`` when ``name`` is set to one of the falsy literals. + + Used by :func:`_is_prefetch_enabled` and + :func:`_is_preclone_enabled` to support the default-ON / opt-out + model (set ``IMPLEMENTER_DISPATCHER_PREFETCH=0`` to explicitly + disable the rich prompt path for one cycle). An unset variable is + NOT falsy here — it falls through to the default. + """ + return os.environ.get(name, "").strip().lower() in _FALSY_ENV_VALUES + + def _is_prefetch_enabled() -> bool: - return _env_truthy(PREFETCH_ENV_VAR) + """Return ``True`` when the dispatcher should use the rich + pre-fetched prompt (PR description, diff, CI status, comments, + linked issues, etc.). + + **Default since 2026-05-10: ON.** The post-mortem of the + PR #30 implementer runs showed that without prefetch the + ``task-implementor`` subagent burns ~3-4 minutes on + ``curl`` + ``read`` calls just to discover the PR's head SHA, + base branch, and changed files — all of which the dispatcher + already has from its own Forgejo calls. The rich prompt + eliminates those turns entirely. The legacy title-only prompt + remains available behind an explicit + ``IMPLEMENTER_DISPATCHER_PREFETCH=0`` opt-out (e.g. for + bisecting a regression against the pre-Phase-4 behaviour). + """ + if _env_falsy_explicit(PREFETCH_ENV_VAR): + return False + return True def _is_preclone_enabled() -> bool: - return _env_truthy(PRECLONE_ENV_VAR) + """Return ``True`` when the dispatcher should pre-clone the PR's + head into ``/tmp/`` and pass the working-copy path to the + worker. + + **Default since 2026-05-10: ON.** Symmetric to + :func:`_is_prefetch_enabled`: the pre-clone eliminates the + ``git-isolator-util`` subagent's clone-then-checkout dance for + ``pr_fix`` work (the typical ~60-100 s of network + git I/O). + The worker's prompt carries the ``## Pre-cloned working copy`` + section with a working ``repo_dir`` path so the worker proceeds + directly to the patch step. Set + ``IMPLEMENTER_DISPATCHER_PRECLONE=0`` to fall back to the + in-session clone via ``git-isolator-util``. + """ + if _env_falsy_explicit(PRECLONE_ENV_VAR): + return False + return True # ─── Legacy prompt (preserved for ``IMPLEMENTER_DISPATCHER_PREFETCH=0``) ────