diff --git a/.opencode/agents/task-implementor.md b/.opencode/agents/task-implementor.md index a54b7f13f..ae559d6d9 100644 --- a/.opencode/agents/task-implementor.md +++ b/.opencode/agents/task-implementor.md @@ -64,6 +64,28 @@ permission: # This is where we edit the permissions on an as-needed per-agent basis "nox *": allow + + # Canonical six-gate wrapper. The script self-bootstraps nox via + # the host's ``uvx`` (or a project venv, or system ``nox``) so the + # worker does NOT need to install anything in its ``/tmp`` clone. + # See the ``quality-gates`` skill for the worker-side decision + # recipe + troubleshooting appendix. Pinned to ``bash tools/...`` + # (the script invocation) rather than ``tools/local_ci_gate.sh *`` + # so the worker can't accidentally drop into a different bash + # shape that bypasses the wrapper. The trailing ``*`` covers all + # supported flags (``--fast``, ``--gate ``, ``--continue``, + # ``--list``). + "bash tools/local_ci_gate.sh *": allow + "bash tools/local_ci_gate.sh": allow + + # ``uvx`` is the fallback nox invocation when no project venv is + # available. Allowed so the wrapper's internal ``uvx nox -e `` + # call passes the permission engine when the worker is in a fresh + # /tmp clone. Pinned to ``uvx --quiet nox *`` (the exact shape the + # wrapper emits) rather than a bare ``uvx *`` to keep the + # allowlist tight. + "uvx --quiet nox *": allow + "git -C /tmp/*": allow # ``cat *`` is for READING files only (e.g. ``cat /tmp/work/file.py``). # A heredoc invocation like ``cat < /tmp/x`` would also match @@ -168,6 +190,15 @@ permission: # depth). "implementer-workspace": allow "implementer-pr-context": allow + + # Deterministic recipe for invoking the six-gate quality wrapper + # (``tools/local_ci_gate.sh``). The skill documents the + # ``--fast`` / full / single-gate / continue-on-fail modes, the + # nox-environment bootstrap chain (system → project venv → uvx), + # and the troubleshooting protocol for each gate's common + # failures. Loaded once at session start before step 5 (Run + # quality gates). + "quality-gates": allow --- # Task: Implementor @@ -254,12 +285,14 @@ The script emits ONE of three signals on stdout (always exit 0): | Stdout | Meaning | What you do | |--------|---------|-------------| | empty (0 bytes) | Dispatcher didn't fetch this section (flag off / fetch failed upstream) | Fall through to the legacy GET / webfetch step | -| `null\n` | Dispatcher fetched successfully and confirmed there is NOTHING (e.g. no Epic) | **DO NOT re-GET.** Proceed as if your legacy GET had returned "no Epic" | +| authoritative-empty (`null\n` / `[]\n` depending on field) | Dispatcher fetched successfully and confirmed there is NOTHING (e.g. no Epic, no active REQUEST_CHANGES reviews, no linked issues) | **DO NOT re-GET.** Proceed as if your legacy GET had returned the same empty value | | anything else | Field value (plain text for `description`/`title`/`diff`/`issue_body`, JSON for everything else) | Use it verbatim | +The exact authoritative-empty byte sequence depends on the field's native shape: `epic` and the plain-text fields (`description`, `title`, `diff`, `issue_body`) emit `null`; list fields (`comments`, `reviews`, `issues`) emit `[]`; `metadata` and `ci` always emit a JSON object when the sentinel exists at all (inspect the embedded `*_completed` / `data_complete` flags to decide whether the value is authoritative). + `` is one of: `description`, `issue_body`, `metadata`, `diff`, `ci`, `comments`, `reviews`, `issues`, `epic`. See the `implementer-pr-context` skill's SKILL.md for the per-field output schema and worked examples. -**Why the three-case contract matters.** The naive "empty vs non-empty" reading would cause you to redundantly re-curl Forgejo every time the dispatcher confirmed a section was empty (e.g. PR has no Epic). The middle case (`null\n`) is the dispatcher's authoritative "I checked and there's nothing" — burn no wallclock fetching what is already known absent. +**Why the three-case contract matters.** The naive "empty vs non-empty" reading would cause you to redundantly re-curl Forgejo every time the dispatcher confirmed a section was empty (e.g. PR has no Epic, no active REQUEST_CHANGES reviews). The middle case (authoritative-empty bytes) is the dispatcher's "I checked and there's nothing" — burn no wallclock fetching what is already known absent. If an operator has explicitly opted out via `IMPLEMENTER_DISPATCHER_PREFETCH=0` / `IMPLEMENTER_DISPATCHER_PRECLONE=0` (typically for a bisect or rollback), the sentinels won't exist and both scripts will return empty stdout — your fallback path takes over automatically. No special handling needed on your side. @@ -267,7 +300,7 @@ This is a **performance** change, not a correctness change: the pre-fetched data #### Procedure: `issue_impl` (New Issue Implementation) -1. **Read the issue.** Run `python3 tools/implementer_pr_context.py read --pr {work_number} --field issue_body` (apply the three-case contract from step 0b — content = use; `null` = issue has no body, proceed; empty = fall through to legacy GET). Then `… --field metadata` for `head_sha` / `base_ref` / etc and `… --field comments` for the issue comments. Only if `issue_body` returns empty stdout, fall through to the legacy GET on `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}` (paginate all comments). +1. **Read the issue.** Run `python3 tools/implementer_pr_context.py read --pr {work_number} --field issue_body` (apply the three-case contract from step 0b — content = use it; `null` = issue has an empty body and dispatcher confirmed it, proceed; empty stdout = fall through to legacy GET). Then `… --field metadata` for `head_sha` / `base_ref` / etc and `… --field comments` for the issue comments (where `[]\n` is "dispatcher confirmed no comments"). Only if `issue_body` returns empty stdout, fall through to the legacy GET on `{forgejo_url}/api/v1/repos/{forgejo_owner}/{forgejo_repo}/issues/{work_number}` (paginate all comments). 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}`. @@ -278,11 +311,19 @@ This is a **performance** change, not a correctness change: the pre-fetched data - Full static typing throughout — no `# type: ignore` - All commands via `nox` — never invoke `pip`, `pytest`, `behave`, or `robot` directly -5. **Run quality gates in order.** Prefer the canonical wrapper, which runs the same six sessions sequentially with structured banners: +5. **Run quality gates in order.** Load the `quality-gates` skill once at session start — it carries the deterministic recipe + troubleshooting appendix. The canonical inner-loop call is: ```bash - tools/local_ci_gate.sh + bash tools/local_ci_gate.sh --fast ``` - The wrapper is equivalent to running the gates one at a time: + That single call runs `lint` → `typecheck` → `unit_tests` → `integration_tests` (skipping `e2e_tests` and `coverage_report` which are slow). The wrapper self-bootstraps nox via the host's `uvx` when no project venv is available, so you do NOT need to install anything in the `/tmp` clone — see the skill for the resolution order. + + Before the FINAL commit, also run the full pass: + ```bash + bash tools/local_ci_gate.sh + ``` + This adds `e2e_tests` and `coverage_report`, which are required for the merge queue. + + Equivalent low-level invocations (do NOT use directly; always go through the wrapper): ```bash nox -e lint nox -e typecheck @@ -291,7 +332,13 @@ This is a **performance** change, not a correctness change: the pre-fetched data nox -e e2e_tests nox -e coverage_report ``` - Use `tools/local_ci_gate.sh --fast` for a tight inner loop (skips `e2e_tests` and `coverage_report`); a final full run is still required before commit. + + For single-gate re-runs after a fix, use: + ```bash + bash tools/local_ci_gate.sh --gate + ``` + + Exit codes: `0` = all passed, `1` = at least one gate failed (see skill's troubleshooting appendix for the per-gate corrective action), `2` = environment broken (no nox invocation resolvable — STOP work and report the script's diagnostic verbatim; do NOT attempt to bootstrap nox yourself). 6. **Fix any failures.** If a gate fails, fix the code and re-run the failing gate (and any that follow it). Repeat until all gates pass. Do not move forward with failing gates. diff --git a/.opencode/skills/implementer-pr-context/SKILL.md b/.opencode/skills/implementer-pr-context/SKILL.md index 85865e1bd..ba7a5d6f7 100644 --- a/.opencode/skills/implementer-pr-context/SKILL.md +++ b/.opencode/skills/implementer-pr-context/SKILL.md @@ -59,7 +59,7 @@ python3 tools/implementer_pr_context.py read --pr {work_number} --field {field} |-------|---------------|----------| | `description` | plain text | step 1 of `pr_fix` / `request_changes_pr` (PR body) | | `title` | plain text | when crafting attempt comments, commit messages | -| `metadata` | JSON | when you need `head_sha`, `head_ref`, `base_ref`, `work_type`, `work_group`, `data_complete` | +| `metadata` | JSON | when you need `head_sha`, `head_ref`, `base_ref`, `work_type`, `work_group`, `data_complete`, or the per-section `completion` map | | `ci` | JSON `{status, checks[], status_completed, detail_completed}` | step 4 of `pr_fix` (CI failure investigation) | | `comments` | JSON list | step 3 of `pr_fix` (PR comments) | | `reviews` | JSON list | step 2 of `request_changes_pr` (active REQUEST_CHANGES reviews) | @@ -76,8 +76,8 @@ code 0: | Stdout | Meaning | What you do | |--------|---------|-------------| -| empty (zero bytes) | The dispatcher didn't fetch this section (e.g. prefetch flag off, or fetch failed upstream — the matching `*_completed` flag was false) | Fall through to your legacy GET / webfetch step. | -| `null\n` | The dispatcher fetched successfully and authoritatively confirmed there is nothing to fetch (e.g. the PR has no Epic reference, no linked issues, no active REQUEST_CHANGES reviews). | **DO NOT re-GET**. The dispatcher already verified the section is empty; act as if you received `{}` / `[]` / "no Epic" from the legacy GET. | +| empty (zero bytes) | The dispatcher didn't fetch this section (prefetch flag off, OR fetch failed upstream — the matching `*_completed` flag was false). | Fall through to your legacy GET / webfetch step. | +| authoritative-empty (`null\n` / `[]\n` / `{}\n` depending on field) | The dispatcher fetched successfully and confirmed there is nothing to fetch (e.g. the PR has no Epic reference, no linked issues, no active REQUEST_CHANGES reviews). | **DO NOT re-GET**. The dispatcher already verified the section is empty; act as if you received the same empty value from the legacy GET. | | any other content | Field value (plain text for `description`/`title`/`diff`/`issue_body`, pretty-printed JSON for everything else). | Use it verbatim — same data shape your legacy GET would have produced. | The three-case contract is critical. The old two-case contract @@ -87,26 +87,146 @@ the worker would redundantly re-curl Forgejo every time the dispatcher had already verified a section was empty. The current contract eliminates that. -**In bash terms**: +### Per-field authoritative-empty signal -```bash -out=$(python3 tools/implementer_pr_context.py read --pr {N} --field epic) -if [ -z "$out" ]; then - # empty stdout — fall through to legacy GET - ... -elif [ "$out" = "null" ]; then - # authoritative "no Epic" — skip the legacy GET - ... -else - # parse $out as the Epic's JSON - ... -fi +Which exact byte sequence signals "dispatcher confirmed empty" +depends on the field's native shape. Plain-text and dict fields +emit `null\n`; list fields emit `[]\n`. Every field has a +completion flag in the sentinel so a legitimately-empty value +(e.g. a PR with no description) is cleanly distinguishable from +a failed fetch. + +| Field | Authoritative-empty bytes | Native shape | +|-------|---------------------------|--------------| +| `description` | `null\n` | plain text | +| `title` | `null\n` | plain text | +| `diff` | `null\n` | plain text | +| `issue_body` | `null\n` | plain text | +| `comments` | `[]\n` | JSON list | +| `reviews` | `[]\n` | JSON list | +| `issues` | `[]\n` | JSON list | +| `epic` | `null\n` | JSON object or null | +| `ci` | always emits a JSON object when sentinel exists | JSON object | +| `metadata` | always emits a JSON object when sentinel exists | JSON object | +| `all` | always emits the full JSON payload when sentinel exists | JSON object | + +For `ci` and `metadata` the worker MUST inspect the embedded +`*_completed` / `data_complete` flags to decide whether the +returned value is authoritative or a partial-failure placeholder +— these fields always emit when the sentinel exists, even if the +underlying fetch failed. + +### Worker decision recipe + +The agent should run the script in a single bash invocation, then +make the routing decision in its own reasoning. **Don't try to +`&&`-chain test commands** — the bash example below is logic, not +a single literal recipe, and the permission allowlist only covers +`python3 tools/implementer_pr_context.py read *`. + +> **Important:** the authoritative-empty token is DIFFERENT for +> each field family. The pseudo-code below shows the `epic` case; +> for other fields **substitute the correct token** before you +> apply the decision: +> +> - `epic`, `description`, `title`, `diff`, `issue_body` → `"null"` +> - `comments`, `reviews`, `issues` → `"[]"` +> - `ci`, `metadata` → always emit a JSON object; inspect the +> embedded `*_completed` / `data_complete` flags instead of +> doing a token comparison. + +Generic pseudo-code (substitute `` per the list +above): + +```text +out = run("python3 tools/implementer_pr_context.py read --pr N --field FIELD") +if out == "": + # dispatcher didn't fetch this section — fall through to legacy GET +elif out == "": + # authoritative "no data" — skip the legacy GET +else: + # parse out as the field value ``` -Plain-text fields (`description`, `title`, `diff`, `issue_body`) -print the raw string with a trailing newline. JSON fields print a -pretty-printed JSON object/list. ``null`` (literal four-byte -JSON null) is reserved for the second case above. +Worked example for `--field epic` (`` = `"null"`): + +```text +out = run("python3 tools/implementer_pr_context.py read --pr 30 --field epic") +if out == "": + # fall through to legacy GET on the parsed Epic number +elif out == "null": + # PR has no Epic — proceed without an Epic body +else: + # parse `out` as the Epic's JSON object +``` + +Worked example for `--field reviews` (`` = `"[]"`): + +```text +out = run("python3 tools/implementer_pr_context.py read --pr 30 --field reviews") +if out == "": + # dispatcher's reviews fetch failed — fall through to /reviews?limit=50 +elif out == "[]": + # PR has no active REQUEST_CHANGES reviews — proceed +else: + # parse `out` as a JSON list of reviews +``` + +### `ci` field exception + +`--field ci` is the one field that does NOT follow the three-case +contract above. It ALWAYS emits a JSON bundle when the sentinel +exists, even if the underlying CI fetch failed. The bundle carries +its own per-section completion booleans the worker must inspect: + +```json +{ + "status": { /* combined-status dict, or null if fetch failed */ }, + "checks": [ /* per-check detail, or [] if not fetched */ ], + "status_completed": true, + "detail_completed": true +} +``` + +Why diverge: CI carries TWO independent completion signals +(`status_completed` for the summary; `detail_completed` for the +per-check enumeration). Collapsing them into one `_MISSING` loses +the case where the status summary succeeded but the per-check +detail failed — the worker still has useful aggregate state ("PR +is red"), it just lacks per-check evidence. + +Writer-side invariant the recipe below relies on: the dispatcher +NEVER emits `status_completed: true` with `status: null`. If +`status_completed` is `true`, then `status` is a real dict and +`status.state` is safe to read. The reverse direction is also +guaranteed — `status_completed: false` always pairs with the +worker falling through before any `status.state` read. This +matches the source of truth in `tools/_implementer_prefetch.py:_fetch_pr_context` +which only flips the flag `True` when `ci_status is not None`. + +Recipe for `--field ci` (same generic pseudo-code style as the +other recipes; substitute `out` with the captured stdout from the +read command): + +```text +out = run("python3 tools/implementer_pr_context.py read --pr 30 --field ci") +bundle = parse_json(out) +if bundle["status_completed"] is not True: + # combined-status fetch failed — curl /commits/{head_sha}/status yourself +elif bundle["status"]["state"] == "success": + # PR is green; no failing checks to triage. (status_completed=True + # guarantees bundle["status"] is a dict — see writer invariant + # above.) +elif bundle["detail_completed"] is not True: + # status was fetched but per-check detail wasn't — curl + # /commits/{head_sha}/statuses for the failing-check URLs +else: + # bundle["checks"] is the authoritative failing-check list +``` + +The same rule applies to `--field metadata` — it always emits a +JSON object and the worker reads the inner `completion` map + +`data_complete` flag to decide what to trust. ## Worked example: `pr_fix` step 1 (Read the PR) @@ -176,8 +296,13 @@ python3 tools/implementer_pr_context.py read --pr 30 --field reviews ``` Returns a JSON list of active REQUEST_CHANGES reviews with each -review's inline comments pre-paginated. Address every concern. If -stdout is empty, fall through to the GET on `/reviews?limit=50`. +review's inline comments pre-paginated. Address every concern. + +- Stdout `[]` → dispatcher confirmed no active REQUEST_CHANGES + reviews. Skip the legacy GET. +- Stdout empty → dispatcher's reviews fetch failed. Fall through + to the GET on `/reviews?limit=50`. +- Otherwise → parse the JSON list and address every concern. ## Worked example: `issue_impl` step 1 (Read the issue) diff --git a/.opencode/skills/quality-gates/SKILL.md b/.opencode/skills/quality-gates/SKILL.md new file mode 100644 index 000000000..833aa2445 --- /dev/null +++ b/.opencode/skills/quality-gates/SKILL.md @@ -0,0 +1,303 @@ +--- +name: quality-gates +description: | + Deterministic recipe for running the six-gate quality check on the + implementer worker side. Use this skill before committing any code + change in a `task-implementor` (or peer) session. It wraps + `tools/local_ci_gate.sh` — the script does its own nox-environment + bootstrap (system nox → project venv → `uvx nox` fallback) so the + worker does NOT need to install or activate anything; just invoke + the wrapper and act on the exit code. + + Load this skill once at agent startup whenever a session is going to + push code. The recipe is short by design: one bash call to run the + fast path during the inner loop, one full call before the final + commit. The troubleshooting appendix lists the 5 most common gate + failures and the exact subsequent bash call that addresses each one. +--- + +# Quality Gates — Deterministic Pre-Commit Recipe + +You are an implementer worker (`task-implementor`, or any agent in +its lineage) about to commit changes. Before pushing, every code +change MUST pass the six canonical quality gates. This skill is the +contract between your prompt and the gate runner — load it once, +follow the recipe verbatim, do NOT improvise. + +## ⚡ One-line summary + +``` +bash tools/local_ci_gate.sh --fast +``` + +That single bash call: + +1. Resolves a nox invocation (system → project venv → `uvx nox`). +2. Runs four gates sequentially: `lint`, `typecheck`, `unit_tests`, + `integration_tests`. +3. Exits `0` if all pass, `1` if any fail, `2` on env / arg error. + +Run again **without** `--fast` (`bash tools/local_ci_gate.sh`) +before the FINAL commit — adds `e2e_tests` and `coverage_report` +which are too slow for the inner loop but required for the merge +queue. + +## Gate inventory + +| Gate | --fast? | Typical wallclock | What it checks | +|---------------------|---------|------------------|----------------| +| `lint` | ✅ | 5–15 s | ruff format + checks across `src/`, `scripts/`, `examples/`, `features/`, `robot/` | +| `typecheck` | ✅ | 30–60 s | pyright across the package + Python tests | +| `unit_tests` | ✅ | 2–4 min | behave BDD suite, parallelised via `behave-parallel` | +| `integration_tests` | ✅ | 3–6 min | Robot Framework integration suite, parallelised via pabot | +| `e2e_tests` | ❌ | 5–10 min | end-to-end Robot suite with real LLM keys — skipped under `--fast` | +| `coverage_report` | ❌ | 1–2 min | coverage rollup from `unit_tests` — skipped under `--fast` | + +Total wallclock for `--fast`: ~5–10 min. Total for the full run: +~15–25 min on a warm `.nox/` cache. + +## Environment bootstrap (read-only — the script handles this) + +You do NOT need to install `nox` or set up a venv manually. The +script resolves the invocation in this order: + +1. `nox` already on `PATH` +2. `${REPO_ROOT}/.venv/bin/nox` (project venv from contributing workflow) +3. `uvx nox` (auto-agents pipeline fallback — `uv` is system-installed) + +The first stderr line of every run announces which path was +selected, e.g.: + +``` +# local_ci_gate.sh — using uvx fallback (nox via /home/drew/.local/bin/uvx) +``` + +If none of the three resolves, the script exits `2` with a +diagnostic listing all three failed paths and the exact one-liner to +install each. **Do NOT attempt to `pip install nox` yourself in the +worker session** — the script's exit code 2 means the host is +mis-provisioned, which is an operator concern, not a worker one. + +## Worker decision recipe + +Use this exact decision tree. Each branch corresponds to one +follow-up bash call; do not interpolate intermediate decisions. + +```text +out = run("bash tools/local_ci_gate.sh --fast") +exit = out.exit_code + +if exit == 0: + # all four fast gates passed; proceed to final full run before commit + out_full = run("bash tools/local_ci_gate.sh") + if out_full.exit_code == 0: + # commit + push via git-commit-util subagent + else: + # one of e2e_tests / coverage_report failed; see troubleshooting + # appendix below +elif exit == 1: + # at least one fast gate failed; the LAST line of stderr names the + # failing gate. Address per the troubleshooting appendix, then re-run + # ONLY the failing gate to confirm fix: + # bash tools/local_ci_gate.sh --gate + # Loop until exit 0, then run --fast again to confirm the upstream + # gates still pass. +elif exit == 2: + # environment is broken (no nox invocation resolvable). Stop work + # and report the script's diagnostic message verbatim in your + # attempt comment. Do NOT attempt to bootstrap nox yourself. +``` + +## Single-gate re-run recipe + +After a fix, re-run only the failing gate to validate quickly +(seconds instead of minutes): + +``` +bash tools/local_ci_gate.sh --gate lint +bash tools/local_ci_gate.sh --gate typecheck +bash tools/local_ci_gate.sh --gate unit_tests +bash tools/local_ci_gate.sh --gate integration_tests +bash tools/local_ci_gate.sh --gate e2e_tests +bash tools/local_ci_gate.sh --gate coverage_report +``` + +`--gate ` MUST match one of the six canonical names exactly. +Misspellings exit `2`. See `bash tools/local_ci_gate.sh --list` for +the live list. + +## Continuing past failures (post-mortem mode) + +When you want every gate's failure listed in one pass (instead of +stopping at the first): + +``` +bash tools/local_ci_gate.sh --continue --fast +``` + +The exit code is still `1` if any gate failed; the final stderr +line names every failing gate. Useful for the first round of fixes +when you'd rather batch them than discover them one at a time. + +## Troubleshooting appendix + +The most frequent gate failures and their corrective actions. +Each entry is "symptom in stderr → exact next bash call". + +### `lint` failure + +**Symptom**: `Found N errors.` from ruff with file paths and rule +codes (e.g. `E501`, `F401`). + +**Action**: For most ruff errors, the autofixer handles it: + +``` +bash tools/local_ci_gate.sh --gate lint +``` + +If errors persist, run ruff format too: + +``` +bash -c "cd /tmp//repo && ${NOX_CMD:-uvx --quiet nox} -e format" +``` + +Then re-run `--gate lint` to confirm. Do NOT manually edit ruff +warnings unless the autofixer left them — `# noqa` directives are a +last resort and require a code-review-grade justification. + +### `typecheck` failure + +**Symptom**: `error:` lines from pyright with file paths and +specific type-mismatch reasons. + +**Action**: pyright errors are NOT auto-fixable. Read the error +location, add the missing type annotation or correct the call shape, +and re-run: + +``` +bash tools/local_ci_gate.sh --gate typecheck +``` + +Do NOT add `# type: ignore` — `CONTRIBUTING.md` explicitly forbids +it. If the type genuinely cannot be expressed (e.g. third-party +library missing stubs), use `cast()` with a comment explaining why. + +### `unit_tests` failure + +**Symptom**: `behave` traceback or a scenario count > 0 in the +failure summary. + +**Action**: The failing scenario name is in the stderr immediately +above the traceback. Re-run only that scenario: + +``` +bash -c "cd /tmp//repo && ${NOX_CMD:-uvx --quiet nox} -e unit_tests -- --tags=@scenario-tag" +``` + +Iterate on the implementation until that scenario passes, then +run the full gate to ensure nothing regressed: + +``` +bash tools/local_ci_gate.sh --gate unit_tests +``` + +### `integration_tests` failure + +**Symptom**: Robot Framework output with `| FAIL |` markers and +`*** Test Cases ***` keyword traces. + +**Action**: Robot failures are often environmental (template DB not +built, fixture not torn down between scenarios). First re-run the +gate ONCE — many integration flakes are transient: + +``` +bash tools/local_ci_gate.sh --gate integration_tests +``` + +If it fails twice in a row, the failure is real. Inspect the Robot +log file written under `output/` for the specific keyword trace +and fix the test or the production code accordingly. + +### `e2e_tests` failure (only in the FULL run) + +**Symptom**: same as `integration_tests` but slower; usually a +real-LLM-key timeout or rate-limit. + +**Action**: e2e failures are almost always environment / network +issues, NOT code defects. Do NOT loop on this gate in your +attempt comment. Report the failure verbatim and let the +supervisor's CI re-run handle it. + +### `coverage_report` failure + +**Symptom**: `coverage: percentage X.X% < required Y.Y%`. + +**Action**: Add unit_tests covering the un-covered lines. The +coverage report's HTML output (under `htmlcov/`) highlights the +exact uncovered lines. After adding tests: + +``` +bash tools/local_ci_gate.sh --gate unit_tests +bash tools/local_ci_gate.sh --gate coverage_report +``` + +## Hard rules + +1. **One bash call per script invocation.** Do not chain with `&&` + or `;` — each gate runs cleanly in its own bash call so the + permission engine can audit it. The script itself handles + internal sequencing. + +2. **Never run `nox` directly.** Always go through + `tools/local_ci_gate.sh` so the bootstrap chain is consistent. + The dispatcher's PR attempt-comment templates expect the script + wrapper's stderr banner format. + +3. **Never manually install nox or uv in the worker session.** + Exit code 2 means the host is mis-provisioned. Report the + script's diagnostic verbatim in your attempt comment. + +4. **Always do BOTH a `--fast` pass and a full pass.** `--fast` + catches 80% of issues in 5–10 min; the full pass catches the + `e2e_tests` / `coverage_report` failures that would otherwise + surface in remote CI 15 minutes after push. + +5. **Trust the resolution banner.** The first stderr line tells + you which nox path the script chose. If the banner shows + `uvx fallback` and the gate is taking longer than usual, that's + the cold-cache provisioning on first call — subsequent calls + in the same session reuse the cache. + +## Working examples + +### Inner loop after a small fix + +``` +bash tools/local_ci_gate.sh --fast +# exit 1 → ruff complained about an unused import in src/foo.py +bash tools/local_ci_gate.sh --gate lint +# exit 0 → re-run the fast suite to confirm nothing else broke +bash tools/local_ci_gate.sh --fast +# exit 0 → ready for the full run +bash tools/local_ci_gate.sh +# exit 0 → commit via git-commit-util +``` + +### First time in a fresh /tmp clone (auto-agents pipeline) + +``` +bash tools/local_ci_gate.sh --fast +# first stderr line: "using uvx fallback (nox via /home/drew/.local/bin/uvx)" +# uv cold-cache + nox session provisioning adds ~30 s to the first +# gate; subsequent gates reuse the cache. +``` + +### Environment is broken + +``` +bash tools/local_ci_gate.sh --fast +# exit 2 → diagnostic block listing the three failed resolution paths. +# Do NOT pip-install nox. Copy the diagnostic verbatim into your +# attempt comment and exit. The supervisor will handle the host +# provisioning issue. +``` diff --git a/CHANGELOG.md b/CHANGELOG.md index 875da44dd..bdcefcf6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,605 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **Quality-gates environment bootstrap + dedicated skill + (2026-05-11).** The Tier 0 task-implementor worker was emitting + "Failed — 'nox' not available in this execution environment" + on every PR because its ``/tmp/...`` throwaway clone had no + Python tooling pre-installed; ``tools/local_ci_gate.sh``'s + pre-flight hard-exited at ``ERROR: nox not on PATH``. Two + changes close the gap. + + - **``tools/local_ci_gate.sh`` self-bootstraps nox.** The + pre-flight now resolves a nox invocation in three steps, + announcing the chosen path on stderr's first line: + + 1. ``nox`` already on ``PATH`` (most human-dev setups). + 2. ``${REPO_ROOT}/.venv/bin/nox`` (project venv created by + the contributing workflow, ``pip install -e .[dev]`` + brings nox in). + 3. ``uvx --quiet nox`` (the auto-agents pipeline fallback — + ``uv`` is system-installed on the host, ``uvx`` provisions + nox per-invocation into uv's tool cache). + + The resolution is captured as a bash array ``NOX_CMD`` and + substituted for every ``nox -e `` call in the gate + loop. If none of the three resolves, the script exits 2 with + a diagnostic listing all three failed paths and the + one-liner installer for each — operators see the gap, the + worker does not attempt to self-install. + + - **New ``quality-gates`` skill** at + ``.opencode/skills/quality-gates/SKILL.md``. Recipe-driven + contract for the implementer worker stack: one-line summary + (``bash tools/local_ci_gate.sh --fast``), gate inventory + with wallclock estimates, decision recipe with exact + follow-up calls for each exit code, single-gate re-run + recipe, troubleshooting appendix for the 5 most common gate + failures (lint, typecheck, unit_tests, integration_tests, + e2e_tests, coverage_report) including the exact next bash + call to address each. Mirrors the ``workspace-isolate`` + skill style — recipe verbatim, no improvisation. + + - **``task-implementor.md`` updated.** Added + ``"bash tools/local_ci_gate.sh *"`` and + ``"uvx --quiet nox *"`` to the bash allowlist so the worker + can invoke the wrapper + its internal uvx fallback. Added + ``"quality-gates": allow`` to the skill allowlist. Step 5 + of the worker procedure now references the skill (load + once at session start, follow recipe verbatim) and + documents the exit-code decision tree inline as a fallback + if the skill load is dropped. + + - **Verified end-to-end against a fresh ``/tmp`` clone.** With + ``PATH=/home/drew/.local/bin:/usr/bin:/bin`` (matching the + auto-agents worker environment): ``--gate lint`` resolved + via ``uvx`` and reported "All checks passed!" in 0 s on a + warm uv cache; ``--gate typecheck`` resolved via ``uvx`` + and reported "0 errors, 3 warnings" in 47 s including + pyright provisioning. The "nox not available" failure from + the 2026-05-11 11:26:56 Tier 0 attempt comment will not + recur. + + **Test additions:** new ``tests/auto_agents/test_local_ci_gate.py`` + with 9 tests pinning the three-step resolution chain, the + bad-shape error paths, and the "system nox failure does not fall + through to uvx" invariant (so a flaky gate's real failure isn't + masked by a fallback re-run). + + **Test results:** 1,170 passing / 3 skipped (1,173 collected). + The +10 delta vs round-7's claimed "1,160 passing" comes from + +9 new tests in ``test_local_ci_gate.py`` plus +1 in + round-7's count (the round-7 entry under-counted by 1 — likely a + parameterised-case expansion landing alongside the round-7 + schema-lock test; the empirical count is what's authoritative). + + **Lint:** verified empirically — 11 pre-existing errors across + the changed files (3 in ``conftest.py``, 3 in + ``_pr_context_sentinel.py``, 4 in ``dispatch_implementer.py``, 1 + in ``implementer_pr_context.py``), unchanged baseline, no new + lints introduced by this round. Supersedes the round-6 / round-7 + entries' "7 pre-existing errors" claim, which was off by 4 — the + ``_to_dict`` refactor did not actually clean up any lints + incidentally (the source of that mis-statement was an aspirational + CHANGELOG draft, not an empirical re-count). + ### Fixed +- **Filesystem-handoff seventh-round refinement: defensive schema-base + collision guard + targeted regression tests for the round-6 + introspection and overlay paths (2026-05-11).** Round 6 left + three P3 observations and one out-of-round E2E suggestion. + This round elevates them to W-items and addresses the + three commit-time risks; the E2E test remains + out-of-round (real Forgejo HTTP fixture) but the + introspection-based reader and overlay-based writer now + have direct regression coverage. No on-disk schema + changes; no behavioural changes — five items, four of + them tests, one a one-line defensive assert in + ``_to_dict``. + + - **W1 (P3 promoted) — Defensive schema-base collision + assert in ``_to_dict``.** Round 6's overlay loop in + ``tools/_pr_context_sentinel.py:_to_dict`` would silently + clobber the value-projection base if a future contributor + added a ``*_completed`` key to BOTH the base AND + ``COMPLETION_FLAG_NAMES``. Added ``assert name not in + projected, ...`` inside the overlay loop with a diagnostic + message naming the offending key and pointing the + contributor to the resolution (remove from one side). + Covered by ``test_to_dict_overlay_rejects_schema_base_collision`` + which monkeypatches the tuple to inject ``"data_complete"`` + (which IS in the value-projection base) and asserts the + assert fires with the expected diagnostic substring. + - **W2 (P3 promoted) — Reader self-adapts to unknown + ``*_completed`` keys (regression test).** Round 6's + introspection-based ``--field metadata`` projection was + tested only by consequence (existing tests exercised it + via realistic payloads). Added + ``test_metadata_projection_self_adapts_to_unknown_completion_keys`` + which writes a sentinel with a ``foo_completed`` key + (not in the current canonical tuple), reads + ``--field metadata``, and asserts ``completion["foo"]`` + is present. Also verifies stable sort order across two + consecutive reads (byte-identical output for operator + diff tooling). + - **W3 (new this round) — Schema-lock test for + ``_to_dict`` output key set.** Top-level on-disk schema + additions / removals were NOT pinned by any single test — + a future contributor could silently broaden or narrow + the sentinel shape without updating SKILL.md or the + worker contract. Added + ``test_to_dict_key_set_is_schema_locked`` which + enumerates the canonical key set as a Python literal + (value keys + derived flags + + ``COMPLETION_FLAG_NAMES``) and asserts set equality + against ``_to_dict`` output. The diagnostic message + instructs the contributor to update SKILL.md, the + worker-side projection, and the CHANGELOG when + intentionally changing the schema. + - **W4 (writer-side typo guard) — Every canonical flag + appears in ``_to_dict`` output.** Round-6's reverse- + direction guard caught typos in ``COMPLETION_FLAG_NAMES`` + against the dataclass; it did NOT catch the case where + the tuple is correct but the overlay loop is bypassed + (typo'd loop variable, accidental ``if`` guard, etc.). + Added ``test_to_dict_projects_every_canonical_completion_flag`` + which constructs a default ``ImplementerPrefetchResult``, + runs ``_to_dict``, and asserts every name in + ``COMPLETION_FLAG_NAMES`` appears as a top-level key. + Diagnostic names the missing keys so the contributor + sees the gap immediately. + - **W5 — CHANGELOG entry (this entry).** + + **Test additions** (+4 net, 1,160 passing total): + + - ``test_pr_context_sentinel.py`` (+3): + ``test_to_dict_key_set_is_schema_locked``, + ``test_to_dict_projects_every_canonical_completion_flag``, + ``test_to_dict_overlay_rejects_schema_base_collision``. + - ``test_implementer_pr_context_cli.py`` (+1): + ``test_metadata_projection_self_adapts_to_unknown_completion_keys``. + + **Out of round:** End-to-end test exercising dispatcher → + fetcher → sentinel write → reader read against a real + Forgejo HTTP fixture. The fetcher-binding tests + + dispatcher integration test together cover ~95% of the + path; a true E2E test would close the remaining 5% but + requires a fixture-driven mock infrastructure currently + scoped to a separate workstream. + + **Test results:** 1,160 passing / 3 skipped (+4 net vs + round 6). Lint: 7 pre-existing errors, no new lints + introduced. + +- **Filesystem-handoff sixth-round refinement: single-source-of-truth + propagated to the sentinel writer + worker reader, bidirectional + drift guard, doc-vs-code alignment (2026-05-11).** A sixth review + surfaced three P1 defence-in-depth gaps in round-5's SoT pattern + plus two P2 CHANGELOG fact-fixes. The Q1 belt-and-braces guard + was unidirectional (caught dataclass→tuple drift, missed + tuple→dataclass typos); the Q1 docstring claimed + ``COMPLETION_FLAG_NAMES`` was consumed by the sentinel writer's + ``_to_dict`` but that claim was aspirational — the actual code + still enumerated 11 explicit ``_read_flag`` calls; and the + worker reader's ``--field metadata`` projection hardcoded the + flag-name list with a stale comment-style enumeration. No + on-disk schema changes; no behavioural changes — all five items + finish propagating the SoT pattern that round 5 introduced. + + - **W2 (P1) — Sentinel writer now consumes + ``COMPLETION_FLAG_NAMES``.** ``tools/_pr_context_sentinel.py`` + imports the tuple at module scope via the established + ``_load_sibling`` pattern and re-exports it as + ``_pr_context_sentinel.COMPLETION_FLAG_NAMES``. ``_to_dict`` + now builds the value-projection base then overlays a + ``*_completed`` entry per name in the tuple — 11 explicit + ``_read_flag`` calls collapsed into one loop. ``title_completed`` + and ``description_completed`` remain explicit derived keys + (they are derived from ``pr_details_completed`` and are NOT + in the canonical dataclass tuple). The Q1 docstring's + aspirational claim is now true. + - **W3 (P1) — Worker reader ``--field metadata`` self-adapts.** + ``tools/implementer_pr_context.py``'s ``_project_field`` + metadata branch was hardcoding the 12-name flag list. Replaced + with a payload introspection + (``[k for k in sorted(payload) if k.endswith("_completed")]``), + so a new flag added to the writer's tuple flows into the + operator's ``--field metadata`` view automatically — without + the reader having to import the prefetch module (preserves + "thin standalone script, no dispatcher imports" invariant + from the module docstring). Sorted enumeration ensures + byte-identical output across consecutive reads for diff + tooling. + - **W1 (P1) — Bidirectional drift guard.** Round-5's + ``test_completion_flag_names_covers_every_dataclass_attribute`` + only checked the dataclass→tuple direction. Empirically: + ``setattr`` silently accepts arbitrary attribute names, so a + typo'd entry in ``COMPLETION_FLAG_NAMES`` (e.g. + ``"ci_detail_complted"``) writes a junk attribute on the + instance while leaving the REAL ``ci_detail_completed`` at + the dataclass default ``True`` — re-introducing + cross-work-type drift for that section without any test + failure. Added a reverse-direction assertion (``tuple_flags - + dataclass_flags == set()``) plus a corrected docstring + explaining both vectors. Verified empirically: simulating a + typo regression now fails on the reverse assertion. + - **W4 (P2) — CHANGELOG factual correction.** Round-5's + summary said "all six items refine the defence-in-depth"; + the actual bullet count is seven (Q1–Q7). Corrected to + "all seven items". + - **W5 (P2) — CHANGELOG "last meta-regression vector" + wording.** Round 5's Q1 claimed it closed "the last + meta-regression vector", but round 6 found another + (reverse-direction tuple drift, W1). Refined the round-5 + wording to "forward-direction (dataclass → tuple) vector" + with a cross-reference to the round-6 entry, so future + readers don't take the round-5 claim as definitive. + + **Test additions** (+0 net, 1,156 passing total): + + - ``test_pr_context_sentinel.py``: + ``test_completion_flag_names_covers_every_dataclass_attribute`` + extended with a reverse-direction assertion (no new test + function — strengthened existing one). + + **Test results:** 1,156 passing / 3 skipped (no count change + vs round 5; the round-6 changes harden existing assertions and + refactor producers without adding new behaviour to cover). Lint: + 7 pre-existing errors (down from 11 baseline; the ``_to_dict`` + refactor cleaned up several stale lints incidentally). + +- **Filesystem-handoff fifth-round refinement: single-source-of-truth + for completion-flag names, fixture deduplication, additional + binding-test guards, contract polish (2026-05-11).** A fifth + review surfaced a meta-regression in the round-4 binding tests + (their hardcoded attribute list could not catch new + ``*_completed`` fields added without preamble support), plus a + cluster of P2 cleanups. No on-disk schema changes; no + behavioural changes — all seven items refine the + defence-in-depth around the cross-work-type drift bug + surfaced and fixed in rounds 3–4. + + - **Q1 (P1) — Single source of truth for completion-flag + attribute names.** Added ``COMPLETION_FLAG_NAMES`` (public + module-level tuple) to ``tools/_implementer_prefetch.py``. + ``_init_completion_flags`` now loops over the tuple + (collapsing 10 explicit assignments into one + ``setattr`` loop), and the fetcher-binding regression + tests in ``test_pr_context_sentinel.py`` import the same + tuple instead of hardcoding the attribute list. Added a + belt-and-braces guard test + (``test_completion_flag_names_covers_every_dataclass_attribute``) + that enumerates every ``*_completed`` attribute on a + default-constructed ``ImplementerPrefetchResult`` and + asserts the tuple covers it. If a future contributor adds + a new ``*_completed`` attribute to the dataclass without + extending the tuple, this guard fails loudly — closing the + forward-direction (dataclass → tuple) meta-regression + vector where the round-4 tests could miss new flags + one-by-one. (Round 6 added a reverse-direction guard for + typo'd / stale tuple entries; see the round-6 entry below.) + - **Q2 — CHANGELOG factual corrections.** Two ``"12 flags"`` + references in the round-4 entry corrected to ``"10 flags"``. + The dataclass has 10 ``*_completed`` attributes; the 12 was + a conflation with the sentinel JSON's 12 derived keys + (10 dataclass attrs + 2 derived ``title_completed`` / + ``description_completed``). + - **Q3 — Fixture deduplication.** Moved the + ``DispatchConfig`` builder logic out of three test files + (``test_dispatch_implementer.py``, ``test_pr_context_sentinel.py``, + ``test_implementer_prompt_snapshot.py``) into a shared + ``conftest.make_dispatch_config(tmp_path, *, dry_run=False)`` + helper. Now every dispatcher-config fixture in the suite + derives from one canonical field list — the same + single-source-of-truth principle round 5's Q1 applied to + completion flags. Three test modules now share one helper; + a future ``DispatchConfig`` field addition only needs to + extend the helper. + - **Q4 + Q5 — SKILL.md ``ci`` recipe polish.** Rewrote the + "ci field exception" recipe in the same generic + pseudo-code style as the other field recipes (``out = run(...)`` + rather than Python-specific ``bundle = json.loads(...)``) + so a worker LLM doesn't trip on a style switch mid-skill. + Also documented the writer-side invariant + (``status_completed=True ⟹ status is not None``) inline + above the recipe, with a cross-reference to the source-of- + truth code path in ``tools/_implementer_prefetch.py:_fetch_pr_context``. + A future contributor who breaks the invariant (e.g. by + flipping the flag True in a code path that doesn't first + assign a non-None status) now has to update the SKILL.md + invariant note too. + - **Q6 — Non-dry-run preamble binding test.** The round-4 + fetcher-binding tests only exercised the dry-run early-exit + path; a pathological regression where someone moves + ``_init_completion_flags()`` INSIDE the ``if cfg.dry_run:`` + block would pass all three. Added + ``test_preamble_runs_before_dryrun_gate_on_non_dryrun_path`` + that stubs ``_review_fetch.fetch_pr_details`` to return + ``None`` on a non-dry-run cfg and asserts + ``pr_details_completed`` is ``False`` (set by the preamble + BEFORE the early-exit). Verified empirically: simulating + the regression sets the flag to the dataclass default + ``True``, failing the assertion. + - **Q7 — CHANGELOG bullet reordering.** Re-ordered the + round-4 R-numbered bullets into sequential order (R1 → R9) + and folded the "R4 subsumed by R5" footnote into R5's + bullet directly, removing the confusing + "subsumed-by-something-listed-elsewhere" reference. + + **Test additions** (+2 net, 1,156 passing total): + + - ``test_pr_context_sentinel.py`` (+2 net): + ``test_completion_flag_names_covers_every_dataclass_attribute`` + (belt-and-braces guard for Q1) and + ``test_preamble_runs_before_dryrun_gate_on_non_dryrun_path`` + (Q6 non-dry-run binding). + - All 1,156 passing; no schema changes; no flake-prone or + network-bound additions. + - Lint baseline unchanged (8 pre-existing in + ``tools/_pr_context_sentinel.py`` / + ``tools/dispatch_implementer.py``, 3 pre-existing in + ``tests/auto_agents/conftest.py``); no new lints + introduced by this round. + +- **Filesystem-handoff fourth-round hardening: P0 fetcher-binding + regression tests, helper unification, dead-code removal, + contract documentation (2026-05-11).** A fourth review surfaced + a P0 test-coverage gap: removing the preamble call from + ``_fetch_pr_context`` silently re-introduced the cross-work-type + drift bug from round 3 while every test still passed. + Empirically verified by stubbing the helper to a no-op. The + third-round changes were functionally correct but unprotected + against regression. None of the changes break the on-disk + schema or backwards-compat. + + - **R1 (P0) — Fetcher-binding regression tests.** Added a new + ``TestFetcherPreambleBinding`` class with three tests that + invoke each public fetcher + (``fetch_pr_fix_context`` / ``fetch_request_changes_context`` / + ``fetch_new_issue_context``) with a dry-run ``DispatchConfig`` + (which short-circuits after the preamble but before any + Forgejo call) and assert every ``*_completed`` flag is False + on the returned result. Removing the preamble line from any + fetcher now fails the corresponding test loudly. Verified + empirically: the same regression simulation that previously + produced 1,146/1,146 green now fails 3 tests. + - **R2 — Pinned-flag assertions in the dispatcher integration + test.** ``test_prefetch_failure_writes_sentinel_with_completion_flags`` + used ``any(v is False for v in completion_flags.values())``, + which became trivially true after the round-3 preamble flips + all 10 flags False up-front. Replaced with named assertions + (``pr_details_completed is True``, ``ci_status_completed is + False``, ``issue_body_completed is False``, etc.) so the + test catches "the wrong flags failed" regressions, not just + "any flag failed". + - **R3 — Stale docstring cross-reference.** The + ``fetch_pr_fix_context`` docstring referenced + ``_set_not_attempted_pr_fix``, a function that never existed + (the real name was ``_set_not_attempted_pr_context``). + Updated to point at ``_init_completion_flags``. + - **R4 + R5 — Helper unification (collapsed dead code into a + single helper).** + ``_set_not_attempted_pr_context(result, include_active_reviews=...)`` + and ``_set_not_attempted_new_issue(result)`` had identical + bodies (both set all 10 dataclass flags False). The + ``include_active_reviews`` parameter had no functional + effect after the round-3 fix — both branches of its + ``if/else`` set the reviews flag False (dead code). + Collapsed both helpers into one + ``_init_completion_flags(result)`` with a comprehensive + docstring explaining the preamble + success-flip pattern; + per-work-type intent moved to the fetcher docstrings. R4 + (the dead-code branch) is subsumed by R5's deletion of the + helper itself. + - **R6 — ``ci`` field contract divergence documented.** Every + other gated field returns ``_MISSING`` (empty stdout) when + the completion flag is False. ``ci`` is the lone exception + — it always emits the bundle and lets the worker inspect + the inner ``status_completed`` / ``detail_completed`` + booleans. Added a "ci field exception" sub-section to + SKILL.md (with a worker recipe for the dual-flag inspection) + and a "Contract divergence (intentional)" block to the + ``_project_field`` ci branch in ``implementer_pr_context.py`` + explaining why. + - **R7 — Parameterized backwards-compat coverage.** The single + ``test_old_sentinel_without_completion_flags_defaults_to_true`` + only exercised ``description`` and ``linked_issues``. + Parameterized across all 7 worker-facing gated fields + (description, title, diff, issue_body, comments, reviews, + issues, epic) so a regression in any field's default-True + fallback now fails immediately. Validates the + ``payload.get(key, True)`` defaults in ``_gated_list`` / + ``_gated_text`` / the inline epic branch are uniform. + - **R8 — ``_read_flag`` scope clarification.** Confirmed + ``data_complete`` already used ``_read_flag`` after round 3; + updated the helper docstring to explicitly list which + attributes it applies to (``*_completed`` flags AND + ``data_complete``) and which it doesn't + (``diff_truncated`` / ``diff_unavailable`` are inverted — + True = bad state — and intentionally use raw + ``bool(getattr(...))``). + - **R9 — pr_details derivation invariant documented.** The + ``_to_dict`` writer projects ``pr_details_completed`` into + three derived keys (``pr_details_completed`` itself, plus + ``title_completed`` / ``description_completed``). Added an + "invariant" comment block explaining why three keys collapse + to one source flag, and the procedure a future contributor + must follow if title / description ever get independent + fetchers (introduce dedicated dataclass attrs and read them + here). + + **Test additions** (+8 net, 1,154 passing total): + + - ``test_pr_context_sentinel.py``: Replaced the three + per-work-type ``TestNotAttemptedFlags`` helper tests with a + single ``test_init_completion_flags_flips_every_flag_false`` + (-2 net) and added the new ``TestFetcherPreambleBinding`` + class with 3 fetcher-level regression tests (+3 net). + - ``test_implementer_pr_context_cli.py``: Expanded the single + backwards-compat test into 8 parameterized cases via + ``@pytest.mark.parametrize`` over every gated field (+7 net). + - All 1,154 passing; no schema changes; no flake-prone or + network-bound additions. + +- **Filesystem-handoff third-round hardening: cross-work-type + completion-flag drift, metadata completion sub-object, strict-True + defensives (2026-05-11).** A follow-up review of the second-round + hardening surfaced a real cross-work-type semantic drift caused by + the dataclass-default-True ``*_completed`` flags. None of the + changes break the on-disk schema for existing sentinels; backwards- + compat for sentinels missing the new flags is verified by test. + + - **Cross-work-type completion-flag drift.** The dataclass + defaulted every ``*_completed`` flag to ``True``, but each + work-type fetcher only attempts a SUBSET of the result's + sections. Result: a worker on ``pr_fix`` work reading + ``--field issue_body`` got ``null\n`` (authoritative-empty + — "fetched and confirmed no body") instead of empty stdout + ("dispatcher didn't try; fall through to legacy GET"). The + workers' documented procedures don't cross work-types in + practice, but the contract was a latent footgun. The fix: + each fetcher now invokes a preamble helper + (``_set_not_attempted_pr_context`` / + ``_set_not_attempted_new_issue``) that flips every flag False + BEFORE any potential early-exit, and the fetcher's success + path explicitly flips them True after each successful section. + The semantic guarantee: ``*_completed=True`` iff the + dispatcher attempted that fetch AND the value is authoritative. + - **Early-exit completion-flag drift (related).** Same root + cause: when ``pr_details`` fetch failed (early-exit), the + subsequent diff / CI / comments fetches weren't attempted but + their flags stayed at the default True. The new preamble + + success-flip pattern handles this uniformly — an early-exit + leaves every not-yet-attempted flag False. + - **``metadata`` field surfaces per-section completion state.** + Previously a worker calling ``--field metadata`` got + ``data_complete`` (the aggregate AND) but not the per-section + flags. The projection now includes a ``completion`` sub-object + mapping every section name to its completion bool, so an + operator inspecting via the metadata field can identify + exactly which sections failed without iterating every + ``--field``. + - **Strict-True defensives in ``_to_dict``.** Replaced 11x + ``bool(getattr(result, "x_completed", True))`` with a + centralised ``_read_flag(result, name)`` helper that only + returns False when the attribute is the literal ``False``. + A typo or accidental None assignment now defaults to True + (the existing safer fallback) instead of silently flipping + to "fetch failed". + - **SKILL.md authoritative-empty token visibility.** The + pseudo-code example used ``--field epic`` (``"null"`` token) + as the only worked example. A reader skimming would + generalise the ``[ "$out" = "null" ]`` check to other fields + incorrectly (list fields use ``"[]"``). The fix adds an + explicit per-field-family substitution block, a generic + template, and a second worked example for ``--field reviews`` + so both authoritative-empty token styles are demonstrated. + + **Test additions** (+12 net, 1,146 passing total): + + - ``test_pr_context_sentinel.py`` (+5): + ``TestNotAttemptedFlags`` class — pins the preamble helper + output for each work_type (``pr_fix``, + ``request_changes_pr``, ``new_issue``) plus two end-to-end + sentinel round-trip tests that catch the drift at write-time. + - ``test_implementer_pr_context_cli.py`` (+7): + ``TestCrossWorkTypeContract`` — pins worker-side reader + behaviour for cross-work-type queries (``pr_fix`` + + ``issue_body`` → empty stdout, ``new_issue`` + ``description`` + → empty stdout, ``issue_impl`` + empty body → ``null\n``, + etc.) plus a backwards-compat round-trip test that exercises + the default-True fallback for sentinels missing the new flags. + - ``test_dispatch_implementer.py`` (refactor): ``test_dry_run_writes_no_sentinel`` + now consumes a new shared ``dry_cfg`` fixture rather than + reconstructing the ``DispatchConfig`` inline. A + ``_make_cfg`` factory keeps the production-shape and dry-run + fixtures in lockstep when future ``DispatchConfig`` fields + land. + +- **Filesystem-handoff second-round hardening: dry-run side-effect, + completion-flag-for-all-fields, doc accuracy (2026-05-11).** A + follow-up review of commit ``0b657cd0`` surfaced one real bug and + several documentation drifts. None changed the on-disk schema in + a backwards-incompatible way (existing sentinels still parse; + worker fallbacks all still trigger correctly). + + - **Dry-run no longer writes a sentinel.** ``_prefetch_prompt`` + in ``dispatch_implementer`` previously called + ``_pr_context_sentinel.write`` unconditionally, so a + ``--dry-run`` cycle (operator-driven preview) left a stale, + empty-shaped ``pr-{N}.json`` in + ``/tmp/cleveragents-implementer-handoff/``. The workspace + sentinel was correctly gated upstream via + ``_build_clone_section``; the PR-context sentinel wasn't. + The fix adds the matching gate. The contract is now uniform: + dry-run produces no on-disk artifacts. + - **Completion flags for plain-text fields.** Pre-fix, the + sentinel schema had no ``description_completed`` / + ``title_completed`` / ``diff_completed`` / ``issue_body_completed`` + flag, so an empty PR description (legitimate) and a failed + ``pr_details`` fetch (transient) produced the same on-disk + state. The worker fell through to a redundant Forgejo GET in + BOTH cases — defeating the perf win for every PR with an empty + description. The fix adds the four flags to + :class:`ImplementerPrefetchResult` and ``_to_dict``; the worker- + side ``_gated_text`` helper now emits ``null\n`` for "fetched, + confirmed empty" so the worker can distinguish it from "didn't + fetch" via the three-case contract. Symmetric with the existing + ``epic_completed`` flag. + - **Documentation accuracy.** The SKILL.md "consume the output" + table claimed `[...]` list-valued fields and the `epic` field + ALL emit ``null\n`` for the authoritative-empty case. They + don't: list fields emit ``[]\n``, ``epic`` emits ``null\n``, + and (now) plain-text fields emit ``null\n``. A worker LLM + following the example's ``[ "$out" = "null" ]`` check for + list-valued fields would never match and would always fall + through to the legacy GET — defeating the perf win in the + confirmed-empty-list case. The fix adds an explicit per-field + output-format table to both ``implementer-pr-context`` SKILL.md + and ``task-implementor.md`` so the contract is byte-exact. + - **Docstring accuracy.** ``_project_field``'s docstring said + "the two-case ``_MISSING`` vs ``None`` distinction" — it's a + three-case distinction (``_MISSING``, ``None``, value). The + fix rewrites the docstring to enumerate every case and to + document why mapping all three cases to ``None`` (the pre- + 2026-05-10 behavior) collapsed the contract. + - **DRY ``_project_field`` repetition.** The 5x repeated + "if not payload.get(`{x}_completed`, True): return _MISSING / + return payload.get(`x`) or []" pattern is now a ``_gated_list`` + helper; the 4x repeated plain-text variant is a ``_gated_text`` + helper. ``_project_field`` shrinks from ~110 lines to ~50 and + adding a new field is a one-liner. + + **Test additions** (+6 net, 1,134 passing total): + + - ``test_dispatch_implementer.py`` (+1): dry-run does not write + the PR-context sentinel. + - ``test_implementer_pr_context_cli.py`` (+5): + - byte-exact contract per field (every field's confirmed-empty + stdout pinned: plain-text → ``null\n``, lists → ``[]\n``, + dicts → JSON object). + - byte-exact no-handoff emits zero bytes for every supported + field (defends against the worker's ``[ -z "$out" ]`` check + drifting from any field). + - ``metadata`` projection round-trips ``data_complete=False`` + and ``error_kinds`` on a partial-fetch failure. + - confirmed-empty description emits ``null\n`` (validates the + new ``description_completed`` path end-to-end). + - failed description fetch emits empty stdout even when the + sentinel happens to carry a stale body. + - ``test_pr_context_sentinel.py`` (+1; 2 migrated to real + ``ImplementerPrefetchResult``): + - completion-flags round-trip-failure-states verifies all + ``*_completed`` flags propagate as False for a totally- + failed prefetch. + - ``test_write_projects_every_field`` and + ``test_write_truncates_large_string_fields`` now use the + real dataclass (catches silent attribute drift across the + writer / dataclass boundary). + - **Filesystem-handoff hardening: three-case read contract, work_type dispatch, `.tmp` orphan cleanup, dead-code removal (2026-05-11).** Post-commit review of the filesystem-handoff feature surfaced two @@ -78,7 +675,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). eliminate. The new wording: the scripts are the SINGLE SOURCE OF TRUTH; prompt sections are documentation only. - **Test additions** (+11 net, 1,128 passing total): + **Test delta** (+5 net, 1,128 passing total). The net total is + smaller than the sum of additions because two helper-test files + shed coverage as their underlying helpers were removed: - ``test_implementer_pr_context_cli.py`` (+5 tests, 26 total): three-case contract (`null` for confirmed-empty Epic, empty for @@ -96,13 +695,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ``clone_handle`` is in the context dict, cleanup runs even with ``context=None``, and partial-fetch failure sets the appropriate ``*_completed=False`` + ``data_complete=False`` flags. - - ``test_workspace_handoff.py``: integration test verifies the - new path doesn't shell out to ``git for-each-ref`` (regression - guard for the dropped helper); empty-``head_ref`` happy path - confirms graceful degradation. - - ``test_implementer_workspace_cli.py``: ``cleanup`` subcommand - tests removed; replaced with a regression guard that asserts - the subcommand stays gone (re-adding it requires also updating + - ``test_workspace_handoff.py`` (-3 net): 4 + ``_resolve_branch_for_sha`` tests dropped with the helper; +1 + regression guard that the new path doesn't shell out to ``git + for-each-ref``; +1 empty-``head_ref`` graceful-degradation test. + - ``test_implementer_workspace_cli.py`` (-4 net): ``cleanup`` + subcommand tests removed alongside the subcommand; +1 + regression guard that asserts the subcommand stays gone + (re-adding it requires also updating ``.opencode/agents/task-implementor.md``). ### Added diff --git a/tests/auto_agents/conftest.py b/tests/auto_agents/conftest.py index ef7d0088c..884269987 100644 --- a/tests/auto_agents/conftest.py +++ b/tests/auto_agents/conftest.py @@ -455,6 +455,48 @@ def pipeline(claim_runtime): return load_tool_module("_review_pipeline", fresh=True) +def make_dispatch_config(tmp_path, *, dry_run: bool = False) -> Any: + """Build a fully-populated implementer ``DispatchConfig`` for + tests. Single source of truth for the field list — keeps the + dispatcher-config-shape consumed by ``test_dispatch_implementer.py``, + ``test_implementer_prompt_snapshot.py``, and + ``test_pr_context_sentinel.py`` (``TestFetcherPreambleBinding``) + from drifting when a new ``DispatchConfig`` field lands + upstream. + + Returns a fresh instance per call — pass distinct ``tmp_path``s + for parallel test runs so the on-disk paths + (``lock_path``, ``heartbeat_path``) don't collide. + + Why a plain helper (not a ``@pytest.fixture``): callers want + BOTH a production-shape cfg (``dry_run=False``) and a dry-run + preview cfg (``dry_run=True``) in the same test module. A + pytest fixture can only return one shape per fixture name; + a parameterless helper composes more cleanly into per-shape + fixtures (``cfg`` / ``dry_cfg``) inside each test file. + """ + runtime = load_tool_module("_dispatch_runtime") + return runtime.DispatchConfig( + token="test-token", + forgejo_url="https://git.example.test", + owner="owner", + repo="repo", + server_url="http://127.0.0.1:4096", + lock_path=tmp_path / "dispatch.lock", + heartbeat_path=tmp_path / "dispatch.heartbeat", + cycle_interval_seconds=1, + max_items_per_cycle=1, + worker_timeout_seconds=30, + claim_ttl_seconds=60, + api_retries=1, + request_timeout_s=5, + script_timeout_seconds=5, + table_name="dispatch_implementer_cycles", + dry_run=dry_run, + cycle_failure_budget=5, + ) + + @pytest.fixture def cfg(tmp_path): """Minimal cfg satisfying ``_claim_runtime.RuntimeContext``. diff --git a/tests/auto_agents/test_dispatch_implementer.py b/tests/auto_agents/test_dispatch_implementer.py index 76043ced4..af407a4fd 100644 --- a/tests/auto_agents/test_dispatch_implementer.py +++ b/tests/auto_agents/test_dispatch_implementer.py @@ -29,6 +29,7 @@ import pytest from .conftest import ( FakeReviewAPI, load_tool_module, + make_dispatch_config, stub_forgejo_ci_detail, stub_forgejo_ci_status, stub_forgejo_diff_via_urlopen, @@ -94,26 +95,28 @@ def fake_implementer_api(driver_chain, monkeypatch): @pytest.fixture def cfg(driver, tmp_path): - runtime = load_tool_module("_dispatch_runtime") - return runtime.DispatchConfig( - token="tok", - forgejo_url="https://git.example.test", - owner="owner", - repo="repo", - server_url="http://127.0.0.1:4096", - lock_path=tmp_path / "impl.lock", - heartbeat_path=tmp_path / "impl.heartbeat", - cycle_interval_seconds=1, - max_items_per_cycle=1, - worker_timeout_seconds=30, - claim_ttl_seconds=60, - api_retries=1, - request_timeout_s=5, - script_timeout_seconds=5, - table_name="dispatch_implementer_cycles", - dry_run=False, - cycle_failure_budget=5, - ) + """Production-shape ``DispatchConfig`` (``dry_run=False``). + + Delegates to :func:`conftest.make_dispatch_config` so the + field list stays in lockstep with every other test module's + dispatcher-config fixture (``test_implementer_prompt_snapshot.py``, + ``test_pr_context_sentinel.py``). Depends on ``driver`` only + to enforce module load-order (the driver chain freshens + ``_dispatch_runtime`` so the cfg's class reference is the + same one the dispatcher imports). + """ + return make_dispatch_config(tmp_path, dry_run=False) + + +@pytest.fixture +def dry_cfg(driver, tmp_path): + """Dry-run dispatcher cfg. Tests that exercise the + operator-visible ``--dry-run`` contract should use this rather + than mutating ``cfg`` — keeps the fixture's scope clean for + parallel test ordering. Same single-source-of-truth helper + as ``cfg``. + """ + return make_dispatch_config(tmp_path, dry_run=True) def _pr_item(number: int = 30, title: str = "Fix login redirect") -> dict[str, Any]: @@ -1217,15 +1220,27 @@ class TestPrContextSentinelIntegration: the sentinel must record ``*_completed=False`` for that section so the worker's reader returns empty stdout (worker falls through to its legacy GET) instead of - serving an empty value as authoritative.""" + serving an empty value as authoritative. + + Pins SPECIFIC flags rather than ``any(v is False)`` — the + round-3 preamble (``_init_completion_flags``) now flips + every flag False before fetching, so the looser + any-False assertion would be trivially satisfied even + if the wrong flags failed. Specificity matters. + """ monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "1") handoff = tmp_path / "pr-context" monkeypatch.setenv( "IMPLEMENTER_DISPATCHER_PR_CONTEXT_DIR", str(handoff) ) - # Only pr_details + diff stubbed; CI/comments/epic fetches - # will hit the FakeReviewAPI's default 404 path and the - # prefetch result will record them as failed. + # Stubs: pr_details + diff explicitly succeed. + # FakeReviewAPI's default is ``200 / []`` so ``pr_comments`` + # (a paginated list endpoint) parses as an empty-but- + # complete page and reports ``completed=True``. Genuine + # parse failures land on ``ci_status`` because + # ``fetch_ci_status`` expects a DICT but the default body + # is a LIST — the dict-coercion failure marks the section + # ``completed=False``. stub_forgejo_pr_details(fake_implementer_api) stub_forgejo_diff_via_urlopen(monkeypatch) item = _pr_item() @@ -1237,18 +1252,74 @@ class TestPrContextSentinelIntegration: assert target.exists() import json payload = json.loads(target.read_text()) - # Some section's completion flag MUST be False — proves the - # dispatcher wrote real completion data rather than the - # all-defaults-True placeholder that the original code - # would have produced for a totally-failed prefetch. - completion_flags = { - k: v for k, v in payload.items() - if k.endswith("_completed") - } - assert any(v is False for v in completion_flags.values()), ( - f"expected at least one *_completed=False on partial " - f"prefetch failure; got {completion_flags}" + # Stubbed-success sections — flag MUST flip True. + assert payload["pr_details_completed"] is True, ( + "pr_details was stubbed to succeed; its flag must be " + "True. False here means the writer is dropping the " + "success signal." ) + assert payload["diff_completed"] is True, ( + "diff was stubbed via urlopen to return a real diff; " + "flag must be True." + ) + # Genuine parse-failure section — flag MUST stay False. + assert payload["ci_status_completed"] is False, ( + "ci_status fetch hit the FakeReviewAPI default (200/[] " + "— a list where _review_fetch.fetch_ci_status expects " + "a dict). The dict-coercion failure marks the section " + "incomplete; flag must be False so the worker falls " + "through to its legacy GET." + ) + # Cross-work-type drift guard: ``pr_fix`` (failing_ci_pr) + # work NEVER attempts these — they must stay False from + # the preamble through to the sentinel. This is the + # round-3 fix; any True here means the preamble silently + # disappeared. + assert payload["issue_body_completed"] is False, ( + "pr_fix doesn't fetch issue_body; flag must remain " + "False from the preamble — regression guard for the " + "cross-work-type drift bug." + ) + assert payload["issue_comments_completed"] is False + assert payload["request_changes_reviews_completed"] is False # ``data_complete`` is the aggregate AND — if any section # failed, the aggregate MUST be False. assert payload["data_complete"] is False + + def test_dry_run_writes_no_sentinel( + self, + driver, + dry_cfg, + monkeypatch, + tmp_path, + ): + """Dry-run is the operator-visible "preview, no I/O" + contract. The dispatcher MUST NOT write a PR-context + sentinel during a dry-run cycle, even with prefetch enabled. + + Regression guard: prior to this fix, the fetchers correctly + returned empty results in dry-run, but ``_prefetch_prompt`` + still called ``_pr_context_sentinel.write`` and produced a + stale empty-shaped sentinel on disk. An operator inspecting + ``/tmp/cleveragents-implementer-handoff/`` after a dry-run + would reasonably conclude the dispatcher had executed when + in fact it had previewed. + """ + monkeypatch.setenv("IMPLEMENTER_DISPATCHER_PREFETCH", "1") + handoff = tmp_path / "pr-context" + monkeypatch.setenv( + "IMPLEMENTER_DISPATCHER_PR_CONTEXT_DIR", str(handoff) + ) + + item = _pr_item() + group = driver.WORK_GROUPS[0] + driver._implementation_prompt_dispatch(dry_cfg, item, group) + + # The handoff directory may not exist at all (sentinel write + # creates parents) — that's the correct "no I/O" outcome. + # If it DID get created, the PR sentinel must not exist. + target = handoff / "pr-30.json" + assert not target.exists(), ( + "dry-run wrote a PR-context sentinel to disk; violates " + "the operator-visible no-I/O preview contract" + ) diff --git a/tests/auto_agents/test_implementer_pr_context_cli.py b/tests/auto_agents/test_implementer_pr_context_cli.py index 2b272cd7b..04f580b68 100644 --- a/tests/auto_agents/test_implementer_pr_context_cli.py +++ b/tests/auto_agents/test_implementer_pr_context_cli.py @@ -55,7 +55,9 @@ def _write_sentinel(handoff_dir, pr_number, **overrides): "created_at": "2026-05-11T00:00:00+00:00", "listing_title": "Test PR", "title": "Test PR title", + "title_completed": True, "description": "Test PR body\nSecond line.", + "description_completed": True, "head_sha": "deadbeef" * 5, "head_ref": "feature/test", "base_ref": "master", @@ -73,6 +75,7 @@ def _write_sentinel(handoff_dir, pr_number, **overrides): ], "request_changes_reviews_completed": True, "issue_body": "Linked issue body for issue_impl", + "issue_body_completed": True, "issue_comments": [], "issue_comments_completed": True, "linked_issues": [{"number": 99, "body": "Issue #99 body"}], @@ -80,6 +83,7 @@ def _write_sentinel(handoff_dir, pr_number, **overrides): "epic": {"number": 42, "body": "Parent Epic body"}, "epic_completed": True, "diff": "--- a/foo.py\n+++ b/foo.py\n@@\n-bar\n+baz\n", + "diff_completed": True, "diff_truncated": False, "diff_unavailable": False, "diff_info": {"added": 1, "removed": 1}, @@ -244,14 +248,40 @@ def test_read_all_returns_full_payload(cli, handoff_dir, capsys): # ─── read: empty / missing fields ──────────────────────────────────────── -def test_read_empty_description_prints_nothing( +def test_read_confirmed_empty_description_prints_null( cli, handoff_dir, capsys ): - """An empty PR description ``""`` is treated as "no description; - fall through" — there's no ``description_completed`` flag to - disambiguate a legitimate empty body from an unfetched one, so - the conservative path is to let the worker GET to verify.""" - _write_sentinel(handoff_dir, 50, description="") + """A PR with an empty body and ``description_completed=True`` + is authoritatively empty — emits ``null\\n`` so the worker + skips the legacy GET. This is the description analogue of the + epic case; ``description_completed`` was added to the sentinel + schema specifically to close this gap (before its addition, + every PR with an empty description cost the worker a redundant + Forgejo GET).""" + _write_sentinel( + handoff_dir, 50, + description="", + description_completed=True, + ) + rc = cli.main(["read", "--pr", "50", "--field", "description"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "null\n" + + +def test_read_failed_description_fetch_prints_nothing( + cli, handoff_dir, capsys +): + """``description_completed=False`` means the ``pr_details`` + fetch failed; worker should fall through to the legacy GET on + ``/pulls/{N}``. Even if ``description`` happened to be + populated (e.g. from a partial response), the False flag is + authoritative.""" + _write_sentinel( + handoff_dir, 50, + description="some stale body", + description_completed=False, + ) rc = cli.main(["read", "--pr", "50", "--field", "description"]) captured = capsys.readouterr() assert rc == 0 @@ -451,3 +481,496 @@ def test_missing_pr_arg_exits_nonzero(cli): with pytest.raises(SystemExit) as exc: cli.main(["read", "--field", "description"]) assert exc.value.code != 0 + + +# ─── byte-exact contract guard ────────────────────────────────────────── + + +def test_byte_exact_contract_per_field(cli, handoff_dir, capsys): + """Locks the exact stdout byte sequence emitted for the + "authoritative-empty" case of every supported field. + + The SKILL.md and ``task-implementor.md`` document a three-case + contract: + + - empty stdout → dispatcher didn't fetch (or fetch failed) + - field-specific authoritative-empty bytes → dispatcher fetched + and confirmed absence + - field value → use it verbatim + + The exact authoritative-empty bytes are **part of the contract** + that worker LLMs read from documentation. A previous critique + found the SKILL.md table claimed all fields emit ``null\\n`` for + the authoritative-empty case when in fact list fields emit + ``[]\\n``. The current contract: plain-text fields and ``epic`` + emit ``null\\n``; list fields emit ``[]\\n``; ``metadata`` and + ``ci`` always emit a JSON object. + + This test pins those byte sequences down so a future change to + ``_project_field`` or ``_emit`` that drifts from the documented + contract fails loudly here, before it ever ships to a worker. + """ + # Construct one sentinel where every section is "fetched + # successfully and confirmed empty": + _write_sentinel( + handoff_dir, 80, + work_type="pr_fix", + description="", + description_completed=True, + title="", + title_completed=True, + diff="", + diff_completed=True, + issue_body="", + issue_body_completed=True, + ci_status=None, + ci_status_completed=True, + ci_detail=[], + ci_detail_completed=True, + pr_comments=[], + pr_comments_completed=True, + request_changes_reviews=[], + request_changes_reviews_completed=True, + linked_issues=[], + linked_issues_completed=True, + epic=None, + epic_completed=True, + ) + + # Plain-text fields: now emit ``null\n`` when the dispatcher + # confirmed an empty body. Pre-2026-05-11 these emitted empty + # stdout because the schema had no completion flag; the flag + # was added specifically to close this gap. + for field in ("description", "title", "diff", "issue_body"): + capsys.readouterr() # reset + rc = cli.main(["read", "--pr", "80", "--field", field]) + captured = capsys.readouterr() + assert rc == 0, field + assert captured.out == "null\n", ( + f"field {field!r}: confirmed-empty plain-text must " + f"emit exactly 'null\\n'; got {captured.out!r}" + ) + + # List fields emit "[]\n" — truthy bytes, distinct from empty + # stdout, so the worker can tell "confirmed empty" apart from + # "didn't fetch". The byte-exact match (including the trailing + # newline) is the contract the SKILL.md documents. + for field in ("comments", "reviews", "issues"): + capsys.readouterr() + rc = cli.main(["read", "--pr", "80", "--field", field]) + captured = capsys.readouterr() + assert rc == 0, field + assert captured.out == "[]\n", ( + f"field {field!r}: list-valued confirmed-empty must " + f"emit exactly '[]\\n'; got {captured.out!r}" + ) + + # ``epic`` emits ``null\n`` for the same reason: it's a JSON + # object (or null), so the natural JSON encoding of "fetched, + # but no Epic reference" is the literal ``null`` token. + capsys.readouterr() + rc = cli.main(["read", "--pr", "80", "--field", "epic"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "null\n", ( + f"field 'epic': confirmed-no-Epic must emit exactly " + f"'null\\n'; got {captured.out!r}" + ) + + # ``metadata`` and ``ci`` always emit a JSON object when the + # sentinel exists, even when the underlying fetch failed — the + # embedded ``*_completed`` flags are the worker's signal. So + # they have NO authoritative-empty bytes; they always look + # populated and the worker MUST inspect the flags. + capsys.readouterr() + rc = cli.main(["read", "--pr", "80", "--field", "metadata"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out.strip().startswith("{") + parsed = json.loads(captured.out) + assert parsed["data_complete"] is True + + capsys.readouterr() + rc = cli.main(["read", "--pr", "80", "--field", "ci"]) + captured = capsys.readouterr() + assert rc == 0 + parsed = json.loads(captured.out) + assert parsed["status_completed"] is True + assert parsed["detail_completed"] is True + + +def test_byte_exact_no_handoff_emits_zero_bytes(cli, handoff_dir, capsys): + """When NO sentinel exists, every field must emit exactly zero + bytes (no trailing newline, no "null", no "[]"). The worker's + ``[ -z "$out" ]`` check is the universal "fall through" signal + and depends on this being byte-exact across every field.""" + for field in ( + "description", "title", "diff", "issue_body", + "metadata", "ci", "comments", "reviews", "issues", + "epic", "all", + ): + capsys.readouterr() + rc = cli.main(["read", "--pr", "999", "--field", field]) + captured = capsys.readouterr() + assert rc == 0, field + assert captured.out == "", ( + f"field {field!r}: missing-handoff must emit exactly " + f"zero bytes; got {captured.out!r}" + ) + + +class TestCrossWorkTypeContract: + """Worker-side reader's three-case contract MUST honour the + fetcher's "not attempted" signal for every field. + + A worker on ``pr_fix`` work who reads ``--field issue_body`` + needs empty stdout (fall through) — not ``null\\n`` + (authoritative-empty). The on-disk sentinel carries + ``issue_body_completed=False`` for ``pr_fix`` work; these + tests verify the reader honours it. + + Without these tests, a future change to the projection logic + could silently drop the completion-flag check and start + serving authoritative-empty signals for sections the + dispatcher never attempted, defeating the perf win for + correctness. + """ + + def test_pr_fix_issue_body_returns_missing( + self, cli, handoff_dir, capsys + ): + """A ``pr_fix`` sentinel marks ``issue_body_completed=False`` + (issue body is not attempted for PR-shaped work). Reader + emits empty stdout so worker falls through to legacy GET.""" + _write_sentinel( + handoff_dir, 100, + work_type="pr_fix", + issue_body="", + issue_body_completed=False, + ) + rc = cli.main(["read", "--pr", "100", "--field", "issue_body"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "", ( + "pr_fix work + not-attempted issue_body must emit " + "empty stdout (fall through), NOT 'null\\n' (which " + "would mislead the worker into skipping the legacy GET)" + ) + + def test_pr_fix_reviews_returns_missing_when_not_attempted( + self, cli, handoff_dir, capsys + ): + """``pr_fix`` (failing-CI bucket) doesn't fetch reviews; + sentinel carries ``request_changes_reviews_completed=False``. + Reader emits empty stdout.""" + _write_sentinel( + handoff_dir, 101, + work_type="pr_fix", + request_changes_reviews=[], + request_changes_reviews_completed=False, + ) + rc = cli.main(["read", "--pr", "101", "--field", "reviews"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "", ( + "pr_fix work + not-attempted reviews must emit empty " + "stdout, NOT '[]\\n' (which would tell the worker the " + "dispatcher fetched reviews and confirmed there were " + "none — false)" + ) + + def test_new_issue_description_returns_missing( + self, cli, handoff_dir, capsys + ): + """A ``new_issue`` sentinel has no PR yet — + ``description_completed=False``. Worker reading + ``--field description`` for issue_impl work falls through + (though the documented procedure doesn't ask for it, + the contract must hold for defensive workers).""" + _write_sentinel( + handoff_dir, 102, + work_type="issue_impl", + description="", + description_completed=False, + title="", + title_completed=False, + ) + rc = cli.main(["read", "--pr", "102", "--field", "description"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "", ( + "new_issue work + not-attempted description must emit " + "empty stdout. The dispatcher never fetched a PR (none " + "exists); the worker must fall through rather than " + "treating the empty string as 'PR has empty body'" + ) + + def test_new_issue_diff_returns_missing( + self, cli, handoff_dir, capsys + ): + """``new_issue`` doesn't fetch a diff (no PR exists). + ``diff_completed=False`` → empty stdout.""" + _write_sentinel( + handoff_dir, 103, + work_type="issue_impl", + diff="", + diff_completed=False, + ) + rc = cli.main(["read", "--pr", "103", "--field", "diff"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "" + + def test_issue_impl_issue_body_returns_content( + self, cli, handoff_dir, capsys + ): + """Flip side: ``issue_impl`` work DID fetch the issue + body, so ``issue_body_completed=True`` and the value + round-trips. This is the positive case proving the + not-attempted guard doesn't accidentally suppress real + data.""" + _write_sentinel( + handoff_dir, 104, + work_type="issue_impl", + issue_body="Implement JWT refresh endpoint.", + issue_body_completed=True, + ) + rc = cli.main(["read", "--pr", "104", "--field", "issue_body"]) + captured = capsys.readouterr() + assert rc == 0 + assert "Implement JWT refresh endpoint" in captured.out + + def test_issue_impl_with_empty_body_emits_null( + self, cli, handoff_dir, capsys + ): + """An issue WITH an empty body, successfully fetched by + ``issue_impl`` work, emits ``null\\n`` — the + authoritative-empty signal. The worker skips the legacy + GET. Distinct from the not-attempted case (above) which + emits empty stdout.""" + _write_sentinel( + handoff_dir, 105, + work_type="issue_impl", + issue_body="", + issue_body_completed=True, # fetched + empty + ) + rc = cli.main(["read", "--pr", "105", "--field", "issue_body"]) + captured = capsys.readouterr() + assert rc == 0 + assert captured.out == "null\n", ( + "issue_impl + fetched + empty body must emit 'null\\n' " + "(authoritative-empty), distinct from the not-attempted " + "case which emits empty stdout" + ) + + +def _old_sentinel_payload() -> dict: + """Build a sentinel WITHOUT any ``*_completed`` flags — the + shape a pre-2026-05-11 dispatcher would have written. Used by + the parameterized backwards-compat tests below to verify every + gated field's default-True path survives missing flags. + + Values are intentionally mixed (some populated, some empty, + list fields explicitly empty) so a single payload exercises + both "field present + populated" and "field present + empty" + paths simultaneously for every field family. + """ + return { + "schema_version": 1, + "pr_number": 200, + "work_type": "pr_fix", + "work_group": "failing_ci_pr", + "dispatcher_pid": 1000, + "created_at": "2026-05-10T00:00:00+00:00", + "listing_title": "Old PR", + # Plain-text fields: one populated, three empty. + "title": "Old PR title", + "description": "Old PR body", + "diff": "--- a\n+++ b\n", + "issue_body": "", + # List fields: one populated, three empty. + "pr_comments": [{"id": 1, "body": "comment"}], + "request_changes_reviews": [], + "issue_comments": [], + "linked_issues": [], + # Dict / scalar. + "ci_status": {"state": "failure"}, + "ci_detail": [], + "epic": None, + "head_sha": "deadbeef", + "head_ref": "old-feat", + "base_ref": "master", + "diff_truncated": False, + "diff_unavailable": False, + "diff_info": {}, + "data_complete": True, + "error_kinds": [], + # Deliberately omitted: every ``*_completed`` flag added + # in the 2026-05-11 hardening. + } + + +# (field, expected_stdout) for every gated field — the worker +# contract for a sentinel without completion flags must keep +# producing these outputs so a rolling deploy doesn't blind every +# in-flight worker. Plain-text empty → ``null\n``; list empty → +# ``[]\n``; populated → content prefix. +_OLD_SENTINEL_FIELD_EXPECTATIONS = [ + # Populated plain-text fields: content verbatim. + ("description", "Old PR body"), + ("title", "Old PR title"), + ("diff", "--- a\n+++ b\n"), + # Populated list field: JSON content (prefix match — pretty- + # printed JSON has a leading "[" that the assertion below + # checks for, with the inner payload as substring). + ("comments", '"id": 1'), + # Empty plain-text fields: default-True → authoritative-empty + # → ``null\n``. This is the byte sequence a worker uses to + # decide "no body, skip the legacy GET". + ("issue_body", "null\n"), + # Empty list fields: default-True → ``[]\n``. + ("reviews", "[]\n"), + ("issues", "[]\n"), + # Empty scalar / dict-or-null field: ``null\n``. + ("epic", "null\n"), +] + + +@pytest.mark.parametrize( + "field,expected", _OLD_SENTINEL_FIELD_EXPECTATIONS, + ids=[f[0] for f in _OLD_SENTINEL_FIELD_EXPECTATIONS], +) +def test_old_sentinel_without_completion_flags_defaults_to_true( + cli, handoff_dir, capsys, field, expected +): + """Backwards-compat: a sentinel written by a pre-2026-05-11 + dispatcher (no ``*_completed`` flags in the JSON payload) + is read with the default-True behavior on EVERY gated field. + + Parameterized across all 7 worker-facing gated fields + (description, title, diff, issue_body, comments, reviews, + issues, epic). The default is intentional — the worker-side + reader treats a missing flag as "fetched successfully" + because that's the only sensible behavior when the schema + doesn't tell us otherwise. + + This exercises the worker's defensive ``payload.get(key, + True)`` default in :func:`_gated_list`, :func:`_gated_text`, + and the inline check in the ``epic`` branch. A future + contributor who replaces any of those ``True`` defaults + with ``False`` would break every old sentinel still on disk + during a rolling deploy — this test catches that. + + ``ci`` and ``metadata`` are deliberately excluded: they + don't follow the three-case contract (always emit a bundle). + Their behavior is verified by dedicated tests above. + """ + import json + payload = _old_sentinel_payload() + (handoff_dir / "pr-200.json").write_text(json.dumps(payload)) + + rc = cli.main(["read", "--pr", "200", "--field", field]) + captured = capsys.readouterr() + assert rc == 0 + # Exact match for the empty cases (those carry the precise + # byte sequence the worker contract relies on); substring + # match for populated cases (pretty-printed JSON / plain + # text both surface the canary token but with surrounding + # formatting). + if expected.endswith("\n") and len(expected) <= 6: + assert captured.out == expected, ( + f"old-sentinel default-True path broken for " + f"--field {field}: expected exact {expected!r}, got " + f"{captured.out!r}" + ) + else: + assert expected in captured.out, ( + f"old-sentinel default-True path broken for " + f"--field {field}: expected substring " + f"{expected!r}, got {captured.out!r}" + ) + + +def test_metadata_projection_self_adapts_to_unknown_completion_keys( + cli, handoff_dir, capsys +): + """The reader's ``--field metadata`` branch must enumerate + payload keys ending in ``_completed`` rather than hardcoding + the canonical name list — so a future writer-side addition + flows into the operator's metadata view without a reader + code change. + + Simulates the regression scenario: a future contributor adds + ``foo_completed`` to the writer's ``COMPLETION_FLAG_NAMES`` + tuple (which the writer auto-projects into the sentinel) and + ships the change without touching the reader. This test + writes such a sentinel directly and verifies the reader picks + up the new key under ``completion["foo"]``. + + Also covers the contract requirement that the projection + output is sorted (byte-identical across consecutive reads + for diff tooling). The previous hardcoded-list reader would + have failed this test because ``foo`` wasn't in its + enumeration; the introspection-based reader passes. + """ + _write_sentinel( + handoff_dir, 82, + # Unknown future flag — not in current canonical tuple. + foo_completed=False, + # Sanity: known flag we expect to appear too. + ci_status_completed=True, + ) + rc = cli.main(["read", "--pr", "82", "--field", "metadata"]) + captured = capsys.readouterr() + assert rc == 0 + parsed = json.loads(captured.out) + completion = parsed["completion"] + assert "foo" in completion, ( + f"Unknown completion key 'foo_completed' was not picked " + f"up by metadata projection — reader is not self-adapting. " + f"Got completion keys: {sorted(completion)}" + ) + assert completion["foo"] is False + assert completion["ci_status"] is True + # Stable sort order: rerun and compare verbatim bytes. + rc2 = cli.main(["read", "--pr", "82", "--field", "metadata"]) + captured2 = capsys.readouterr() + assert rc2 == 0 + assert captured.out == captured2.out, ( + "metadata projection is not byte-stable across consecutive " + "reads — operator diff tooling will produce spurious changes." + ) + + +def test_metadata_reflects_partial_fetch_failure( + cli, handoff_dir, capsys +): + """``metadata`` always emits a JSON object when the sentinel + exists, regardless of which sections failed. The worker is + documented to inspect ``data_complete`` to decide whether the + metadata is fully authoritative — this test verifies that + ``data_complete=False`` survives the projection round-trip so + the worker actually sees it.""" + _write_sentinel( + handoff_dir, 81, + ci_status_completed=False, + pr_comments_completed=False, + data_complete=False, + error_kinds=["ci-status-fetch-failed", "pr-comments-paginate-failed"], + ) + rc = cli.main(["read", "--pr", "81", "--field", "metadata"]) + captured = capsys.readouterr() + assert rc == 0 + parsed = json.loads(captured.out) + assert parsed["data_complete"] is False + assert "ci-status-fetch-failed" in parsed["error_kinds"] + assert "pr-comments-paginate-failed" in parsed["error_kinds"] + # The ``completion`` sub-object surfaces per-section failure + # state so an operator inspecting metadata can identify WHICH + # sections failed without having to iterate every --field. + assert parsed["completion"]["ci_status"] is False + assert parsed["completion"]["pr_comments"] is False + # Sections that succeeded should be True (defaults from + # _write_sentinel). + assert parsed["completion"]["description"] is True + assert parsed["completion"]["epic"] is True diff --git a/tests/auto_agents/test_implementer_prompt_snapshot.py b/tests/auto_agents/test_implementer_prompt_snapshot.py index dbf05aa52..e007cec3c 100644 --- a/tests/auto_agents/test_implementer_prompt_snapshot.py +++ b/tests/auto_agents/test_implementer_prompt_snapshot.py @@ -26,7 +26,7 @@ from pathlib import Path import pytest -from .conftest import load_tool_module +from .conftest import load_tool_module, make_dispatch_config _REPO_ROOT = Path(__file__).resolve().parents[2] @@ -48,26 +48,13 @@ def driver_chain(): @pytest.fixture def cfg(tmp_path): - runtime = load_tool_module("_dispatch_runtime") - return runtime.DispatchConfig( - token="t", - forgejo_url="https://git.example.test", - owner="owner", - repo="repo", - server_url="http://127.0.0.1:4096", - lock_path=tmp_path / "d.lock", - heartbeat_path=tmp_path / "d.hb", - cycle_interval_seconds=1, - max_items_per_cycle=1, - worker_timeout_seconds=30, - claim_ttl_seconds=60, - api_retries=1, - request_timeout_s=5, - script_timeout_seconds=5, - table_name="dispatch_implementer_cycles", - dry_run=True, - cycle_failure_budget=5, - ) + """Dispatcher cfg via the shared conftest helper. This + module's snapshots assert prompt structure under the + dispatcher's preview path, so ``dry_run=True`` is the + correct shape — the fetchers and worker dispatch are + deliberately bypassed. + """ + return make_dispatch_config(tmp_path, dry_run=True) @pytest.fixture(autouse=True) diff --git a/tests/auto_agents/test_local_ci_gate.py b/tests/auto_agents/test_local_ci_gate.py new file mode 100644 index 000000000..28f3155a7 --- /dev/null +++ b/tests/auto_agents/test_local_ci_gate.py @@ -0,0 +1,380 @@ +"""Unit tests for ``tools/local_ci_gate.sh`` nox-invocation bootstrap. + +The script's behaviour matters to two callers: + +- Humans running ``tools/local_ci_gate.sh`` in their dev shell, where + ``nox`` is usually pre-installed via ``uv tool install nox`` or + ``pipx``. +- The auto-agents ``task-implementor`` worker, which materialises a + fresh ``/tmp/...`` clone and has **no** ``nox`` installed in that + clone. The worker relies on the script's ``uvx`` fallback. + +These tests pin the three-step resolution chain and the bad-shape +error paths so a future "simplification" cannot silently drop the +``uvx`` fallback and re-introduce the round-1 Tier-0 environment +failure (the worker emitting "nox not available" and skipping every +quality gate). +""" +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "tools" / "local_ci_gate.sh" + + +# ─── Helpers ───────────────────────────────────────────────────────────── + + +def _stub_executable(path: Path, name: str, body: str) -> Path: + """Create a fake executable named ``name`` under ``path`` that + prints / behaves like ``body`` when invoked. Used to simulate + a working ``nox`` / ``uvx`` on a controlled ``PATH`` without + pulling in the real binary. + + The stub records its argv to ``/.log`` so tests can + assert which invocation shape the script picked. The body is + expected to be a single-line shell snippet that produces the + desired exit code (e.g. ``"exit 0"``). + """ + exe = path / name + exe.write_text( + f'#!/usr/bin/env bash\n' + f'echo "$@" >> "{exe}.log"\n' + f'{body}\n' + ) + exe.chmod(0o755) + return exe + + +def _run_script( + *args: str, + env_path: str, + cwd: Path | None = None, + script: Path | None = None, +) -> subprocess.CompletedProcess[str]: + """Run ``local_ci_gate.sh`` with a controlled ``PATH``. Returns + the completed process; tests assert on stdout / stderr / exit. + + The ``script`` argument lets a test pass a copy of the script + placed inside a fake repo so the script's ``REPO_ROOT`` resolves + to the fake repo (not the real workspace root). Required for any + test that needs to check the ``.venv/bin/nox`` resolution path + or otherwise wants ``REPO_ROOT`` redirected. + + We deliberately do NOT inherit the host ``PATH`` — the bootstrap + chain is path-sensitive, and pulling in the host's + ``/home//.local/bin`` would short-circuit half the test + matrix. + """ + env = { + "PATH": env_path, + "HOME": os.environ.get("HOME", "/tmp"), + } + return subprocess.run( + ["bash", str(script or SCRIPT), *args], + capture_output=True, + text=True, + env=env, + cwd=cwd or REPO_ROOT, + timeout=30, + ) + + +# ─── Resolution chain tests ────────────────────────────────────────────── + + +def test_resolution_order_system_nox_wins(tmp_path): + """When ``nox`` is on ``PATH``, the script must use it directly + (NOT fall through to the project venv or ``uvx``). + + The chosen path is announced on stderr's first line; we assert + on that banner. The stub's exit-1 short-circuits the actual + gate body so the test runs in milliseconds. + """ + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + _stub_executable(fake_bin, "nox", "exit 0") + # Also place a fake uvx — the test asserts the script picks nox + # first, NOT uvx. + _stub_executable(fake_bin, "uvx", "exit 0") + + result = _run_script( + "--gate", "lint", + env_path=f"{fake_bin}:/usr/bin:/bin", + ) + assert result.returncode == 0, ( + f"expected exit 0, got {result.returncode}; " + f"stderr:\n{result.stderr}" + ) + # First-line banner names the path chosen. + assert "using system nox" in result.stderr, ( + f"expected 'using system nox' banner, got stderr:\n" + f"{result.stderr}" + ) + # Concrete proof: the nox stub was invoked, uvx was NOT. + assert (fake_bin / "nox.log").exists() + assert not (fake_bin / "uvx.log").exists(), ( + "uvx stub was invoked even though nox was on PATH; " + "the resolution chain is wrong." + ) + + +def test_resolution_order_project_venv_used_when_no_system_nox(tmp_path): + """When system ``nox`` is absent but ``${REPO_ROOT}/.venv/bin/nox`` + exists, the script must use the venv binary (NOT fall through + to ``uvx``). + + This is the human-dev "I created a project venv and pip-installed + nox" path. The test runs the script from a temporary cwd that + holds a ``.venv/bin/nox`` stub so we don't perturb the real repo. + """ + fake_repo = tmp_path / "fake-repo" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + fake_script = fake_repo / "tools" / "local_ci_gate.sh" + shutil.copy(SCRIPT, fake_script) + venv_bin = fake_repo / ".venv" / "bin" + venv_bin.mkdir(parents=True) + _stub_executable(venv_bin, "nox", "exit 0") + + # Also place uvx on PATH — the test asserts the venv binary wins. + other_bin = tmp_path / "other-bin" + other_bin.mkdir() + _stub_executable(other_bin, "uvx", "exit 0") + + result = _run_script( + "--gate", "lint", + env_path=f"{other_bin}:/usr/bin:/bin", + cwd=fake_repo, + script=fake_script, + ) + assert result.returncode == 0, ( + f"expected exit 0, got {result.returncode}; " + f"stderr:\n{result.stderr}" + ) + assert "using project venv nox" in result.stderr + assert (venv_bin / "nox.log").exists() + assert not (other_bin / "uvx.log").exists() + + +def test_resolution_order_uvx_fallback_used_in_fresh_clone(tmp_path): + """When neither system ``nox`` nor a project venv is available, + the script must fall through to ``uvx nox``. This is the + auto-agents pipeline scenario — the worker's ``/tmp/...`` clone + has no Python tooling pre-installed but the host's ``uvx`` is + available. + + Regression guard for the round-1 Tier-0 environment failure + where the worker emitted "nox not available" and skipped every + quality gate. + """ + fake_repo = tmp_path / "fake-clone" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + # No .venv inside fake_repo and no nox on PATH. + + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + _stub_executable(fake_bin, "uvx", "exit 0") + + result = _run_script( + "--gate", "lint", + env_path=f"{fake_bin}:/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 0, ( + f"expected exit 0, got {result.returncode}; " + f"stderr:\n{result.stderr}" + ) + assert "using uvx fallback" in result.stderr, ( + f"expected 'using uvx fallback' banner — " + f"the round-1 regression vector. " + f"Got stderr:\n{result.stderr}" + ) + # uvx was invoked with ``--quiet nox -e `` shape. + log = (fake_bin / "uvx.log").read_text().strip() + assert log == "--quiet nox -e lint", ( + f"expected uvx invocation shape '--quiet nox -e lint', " + f"got '{log}'. Changing the shape requires also updating the " + f"task-implementor.md allow-rule 'uvx --quiet nox *'." + ) + + +def test_resolution_failure_when_no_path_resolves(tmp_path): + """When NONE of the three paths resolves, the script must exit 2 + with a diagnostic that names all three failed paths and lists + the one-liner fix for each. + + Exit code 2 = "environment broken, operator concern, not a + worker concern" — the ``quality-gates`` skill instructs the + worker to copy the diagnostic verbatim into its attempt comment + and stop work. + """ + fake_repo = tmp_path / "fake-empty" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + + # Empty PATH (only system bins). No nox, no venv, no uvx. + result = _run_script( + "--gate", "lint", + env_path="/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 2, ( + f"expected exit 2 for env-broken, got {result.returncode}; " + f"stderr:\n{result.stderr}" + ) + # All three paths must be named in the diagnostic so the operator + # can see WHY each one failed. + assert "nox on PATH" in result.stderr + assert ".venv/bin/nox" in result.stderr + assert "uvx on PATH" in result.stderr + # And the three one-liner fixes are spelled out. + assert "uv tool install nox" in result.stderr + assert "astral.sh/uv/install.sh" in result.stderr + assert "python3 -m venv .venv" in result.stderr + + +# ─── Argument-handling tests (regression guards) ──────────────────────── + + +def test_list_does_not_require_nox(tmp_path): + """``--list`` prints the canonical gate list and exits 0 BEFORE + the pre-flight nox resolution runs. This is load-bearing for + operators / CI tooling that wants to enumerate the gate set on + a host where nox isn't installed. + """ + fake_repo = tmp_path / "fake-empty" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + + result = _run_script( + "--list", + env_path="/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 0, ( + f"--list should not require nox; got exit " + f"{result.returncode}\nstderr:\n{result.stderr}" + ) + assert "lint" in result.stdout + assert "typecheck" in result.stdout + assert "coverage_report" in result.stdout + # First-line resolution banner must NOT appear — pre-flight is + # skipped on the --list path. + assert "using" not in result.stderr.split("\n")[0] + + +def test_help_does_not_require_nox(tmp_path): + """``--help`` prints usage and exits 0 BEFORE pre-flight. Same + rationale as ``--list``.""" + fake_repo = tmp_path / "fake-empty" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + + result = _run_script( + "--help", + env_path="/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 0 + assert "--fast" in result.stdout + assert "--gate" in result.stdout + + +def test_unknown_arg_exits_2_before_pre_flight(tmp_path): + """Unknown arguments exit 2 BEFORE the pre-flight runs, so a + typo doesn't waste a uvx provisioning cycle. The diagnostic + names the offending arg. + """ + fake_repo = tmp_path / "fake-empty" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + + result = _run_script( + "--bogus", + env_path="/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 2 + assert "--bogus" in result.stderr + + +def test_unknown_gate_name_exits_2(tmp_path): + """``--gate `` exits 2 with a diagnostic naming the bad + gate. Tests that the catch happens at argument-parse time, NOT + after the (potentially slow) nox bootstrap. + """ + fake_repo = tmp_path / "fake-empty" + fake_repo.mkdir() + (fake_repo / "tools").mkdir() + shutil.copy(SCRIPT, fake_repo / "tools" / "local_ci_gate.sh") + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + _stub_executable(fake_bin, "nox", "exit 0") + + result = _run_script( + "--gate", "totally-not-a-gate", + env_path=f"{fake_bin}:/usr/bin:/bin", + cwd=fake_repo, + ) + assert result.returncode == 2, ( + f"expected exit 2 for unknown gate, got {result.returncode}; " + f"stderr:\n{result.stderr}" + ) + assert "unknown gate" in result.stderr + assert "totally-not-a-gate" in result.stderr + # The nox stub must NOT have been invoked — argument validation + # runs before pre-flight resolution. + assert not (fake_bin / "nox.log").exists(), ( + "argument validation should happen before nox resolution; " + "the nox stub was invoked despite the bad gate name." + ) + + +# ─── Sequencing test (the bootstrap chain MUST stop at first hit) ──────── + + +def test_resolution_does_not_fall_through_after_system_nox_fails( + tmp_path, +): + """When system ``nox`` is on PATH but FAILS at runtime (e.g. + exit 1 from the gate command), the script must NOT then try + ``uvx`` as a fallback. Falling through would mask the real + failure and double the wallclock for every flaky gate. + + The pre-flight resolves the invocation ONCE; from that point on, + each gate either passes or fails using the chosen invocation. + """ + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + # System nox exits non-zero — simulates a real test failure. + _stub_executable(fake_bin, "nox", "exit 7") + _stub_executable(fake_bin, "uvx", "exit 0") + + result = _run_script( + "--gate", "lint", + env_path=f"{fake_bin}:/usr/bin:/bin", + ) + # Script exits 1 (gate failed), NOT 0 (uvx fallback "saved" it). + assert result.returncode == 1, ( + f"expected exit 1 (gate failed), got {result.returncode}. " + f"If uvx was invoked as a fallback, that's the regression." + ) + # The system nox was tried. + assert (fake_bin / "nox.log").exists() + # The uvx fallback was NOT tried. + assert not (fake_bin / "uvx.log").exists(), ( + "uvx was invoked as a fallback after system nox failed at " + "runtime — this masks real gate failures and is the regression " + "this test guards against." + ) diff --git a/tests/auto_agents/test_pr_context_sentinel.py b/tests/auto_agents/test_pr_context_sentinel.py index 81d7bead3..655d95182 100644 --- a/tests/auto_agents/test_pr_context_sentinel.py +++ b/tests/auto_agents/test_pr_context_sentinel.py @@ -20,7 +20,7 @@ import json import pytest -from .conftest import load_tool_module +from .conftest import load_tool_module, make_dispatch_config @pytest.fixture @@ -38,17 +38,24 @@ def handoff_dir(tmp_path, monkeypatch): class _FakeResult: """Stand-in for ``ImplementerPrefetchResult`` — duck-typed via the - same attribute names the projection reads. Using a plain class - rather than the real dataclass keeps the test independent of - field-order changes in :mod:`_implementer_prefetch`. + same attribute names the projection reads. Used only for tests + that need to force specific failure paths (e.g. forced + ``json.dumps`` failure). For ordinary tests that exercise the + happy-path projection, prefer the real + :class:`ImplementerPrefetchResult` (see + :func:`test_real_prefetch_result_round_trip` and the migrated + happy-path tests) — that catches the silent-attribute-miss + failure mode where someone renames a dataclass field. """ def __init__(self, **kw): self.head_sha = kw.pop("head_sha", "") self.pr_details = kw.pop("pr_details", None) + self.pr_details_completed = kw.pop("pr_details_completed", True) self.diff_text = kw.pop("diff_text", "") self.diff_truncated = kw.pop("diff_truncated", False) self.diff_unavailable = kw.pop("diff_unavailable", False) + self.diff_completed = kw.pop("diff_completed", True) self.diff_info = kw.pop("diff_info", {}) self.ci_status = kw.pop("ci_status", None) self.ci_status_completed = kw.pop("ci_status_completed", True) @@ -61,6 +68,7 @@ class _FakeResult: "request_changes_reviews_completed", True ) self.issue_body = kw.pop("issue_body", "") + self.issue_body_completed = kw.pop("issue_body_completed", True) self.issue_comments = kw.pop("issue_comments", []) self.issue_comments_completed = kw.pop( "issue_comments_completed", True @@ -117,8 +125,15 @@ def test_write_projects_every_field(sentinel, handoff_dir): """Every attribute of the ``ImplementerPrefetchResult`` must appear in the sentinel — failure here means the worker's ``--field`` query for that attribute will return empty (silent - information loss). Pinned shape keeps the contract stable.""" - result = _FakeResult( + information loss). + + Uses the REAL :class:`ImplementerPrefetchResult` (not the + ``_FakeResult`` shim) so a future field-rename on the dataclass + that the writer's ``_to_dict`` forgets to track fails this test + at write-time rather than at worker-runtime. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult( head_sha="sha1", pr_details={"title": "T", "body": "Body", "head": {"ref": "h"}, "base": {"ref": "b"}}, @@ -163,14 +178,139 @@ def test_write_projects_every_field(sentinel, handoff_dir): assert payload["epic"] == {"number": 42} assert payload["data_complete"] is False assert payload["error_kinds"] == ["pr_details:404"] + # Completion flags introduced 2026-05-11 — defending against + # a future writer that forgets to project one. + assert payload["description_completed"] is True + assert payload["title_completed"] is True + assert payload["diff_completed"] is True + assert payload["issue_body_completed"] is True + + +def test_to_dict_key_set_is_schema_locked(sentinel): + """Schema-lock: the exact set of top-level keys emitted by + ``_to_dict`` is asserted here. Adding or removing a key + without updating this test (and the SKILL.md / worker + contract, and the CHANGELOG) fails loudly. + + Rationale: on-disk schema additions are NOT covered by + ``schema_version`` bumps alone — the worker-side reader's + per-field projections silently degrade if a key it expects + disappears, and a new key added without documentation + surprises future maintainers chasing ``--field metadata`` + output diffs. This test is the canonical list; the writer's + ``_to_dict``, the reader's per-field projections, and + SKILL.md must all stay in sync with it. + + NOTE: ``schema_version`` / ``pr_number`` / ``work_type`` / + ``work_group`` / ``dispatcher_pid`` / ``created_at`` / + ``listing_title`` are added by :func:`write` (not + ``_to_dict``) so they are NOT in this set. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult() + out = sentinel._to_dict(result) + + expected_value_keys = { + "title", "description", "head_sha", "head_ref", "base_ref", + "ci_status", "ci_detail", "pr_comments", + "request_changes_reviews", "issue_body", "issue_comments", + "linked_issues", "epic", "diff", + "diff_truncated", "diff_unavailable", "diff_info", + "data_complete", "error_kinds", + } + expected_derived_flags = {"title_completed", "description_completed"} + expected_canonical_flags = set(sentinel.COMPLETION_FLAG_NAMES) + expected_all = ( + expected_value_keys + | expected_derived_flags + | expected_canonical_flags + ) + + actual = set(out.keys()) + missing = expected_all - actual + extra = actual - expected_all + assert not missing, ( + f"_to_dict output is missing schema-locked keys: " + f"{sorted(missing)}. If this was an intentional removal, " + f"update this test, the SKILL.md contract, the worker-side " + f"projection, and CHANGELOG.md." + ) + assert not extra, ( + f"_to_dict output has unexpected new keys: {sorted(extra)}. " + f"If this was an intentional addition, update this test, " + f"SKILL.md, the worker-side projection in " + f"tools/implementer_pr_context.py, and CHANGELOG.md." + ) + + +def test_to_dict_projects_every_canonical_completion_flag(sentinel): + """Writer-side typo guard: every name in + ``COMPLETION_FLAG_NAMES`` must appear as a top-level key in + ``_to_dict`` output. Catches the case where a future + contributor adds a flag to the tuple but the overlay loop is + bypassed (e.g. by a typo'd loop variable, or by guarding the + overlay behind a condition that doesn't fire under default + construction). + + Counterpart to + :class:`TestNotAttemptedFlags.test_completion_flag_names_covers_every_dataclass_attribute` + which guards the dataclass/tuple link. This guards the + tuple/sentinel-output link. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult() + out = sentinel._to_dict(result) + missing = [ + name for name in sentinel.COMPLETION_FLAG_NAMES + if name not in out + ] + assert not missing, ( + f"COMPLETION_FLAG_NAMES entries absent from _to_dict " + f"output: {missing}. The overlay loop in _to_dict did " + f"NOT project them — check for a typo'd loop variable " + f"or a conditional guard." + ) + + +def test_to_dict_overlay_rejects_schema_base_collision( + sentinel, monkeypatch +): + """W1 belt-and-braces: if a future contributor adds a + ``*_completed`` key to the value-projection base AND leaves + the same name in ``COMPLETION_FLAG_NAMES``, the overlay loop + would silently clobber the base value. The new ``assert + name not in projected`` defence fires instead. + + Simulates the regression by monkeypatching + ``COMPLETION_FLAG_NAMES`` to include a name already in the + base dict (``data_complete`` — which IS in the base + projection but NOT currently in the canonical tuple). + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult() + # Inject a colliding name into the canonical tuple. + poisoned = (*sentinel.COMPLETION_FLAG_NAMES, "data_complete") + monkeypatch.setattr(sentinel, "COMPLETION_FLAG_NAMES", poisoned) + + with pytest.raises(AssertionError) as excinfo: + sentinel._to_dict(result) + assert "data_complete" in str(excinfo.value) + assert "silently clobber" in str(excinfo.value).lower() or ( + "schema-base" in str(excinfo.value).lower() + ) def test_write_truncates_large_string_fields(sentinel, handoff_dir): """The 200 KB-per-field cap stops a pathological PR body / diff from bloating the sentinel. Verifies truncation marker is - present so the worker-side can detect it.""" + present so the worker-side can detect it. + + Uses the real :class:`ImplementerPrefetchResult` to ensure the + truncation projection survives a future field-rename. + """ + prefetch = load_tool_module("_implementer_prefetch") huge_body = "x" * 300_000 # 300 KB - above the cap - result = _FakeResult( + result = prefetch.ImplementerPrefetchResult( head_sha="sha", pr_details={"title": "T", "body": huge_body, "head": {"ref": "h"}, "base": {"ref": "b"}}, @@ -187,6 +327,435 @@ def test_write_truncates_large_string_fields(sentinel, handoff_dir): assert "truncated" in desc +class TestNotAttemptedFlags: + """Cross-work-type semantic drift guard. + + Background: each work_type's fetcher attempts a different + subset of sections. ``pr_fix`` never fetches ``issue_body``; + ``new_issue`` never fetches ``pr_details`` / diff / CI / PR + comments. The dataclass defaults all ``*_completed`` flags to + ``True``, which would mislead the worker into reading + "fetched and confirmed empty" for sections the dispatcher + never attempted. + + The fix: each work-type fetcher calls a single preamble + helper :func:`_init_completion_flags` that flips EVERY flag + False BEFORE any potential early-exit, then the fetcher's + success path flips True only the sections it actually + succeeded in fetching. The result: the worker reading a + non-attempted field via the worker-side three-case contract + gets empty stdout (fall through to legacy GET) rather than + ``[]\\n`` / ``null\\n`` (authoritative-empty). + + These tests pin the helper output AND verify the fetchers + actually call it (see ``TestFetcherPreambleBinding`` below) + so a future contributor can't silently drop the preamble. + """ + + def test_init_completion_flags_flips_every_flag_false(self): + """The unified preamble must flip every ``*_completed`` + flag to ``False``. Sanity-check: defaults are ``True``; + post-helper, every flag is ``False``. + + Drives the assertion off ``COMPLETION_FLAG_NAMES`` so a + future ``*_completed`` attribute added to the dataclass + is automatically covered — provided the contributor also + adds the name to that tuple (the single source of truth + the helper itself loops over). If a contributor adds a + dataclass attr WITHOUT extending the tuple, the + belt-and-braces guard + ``test_completion_flag_names_covers_every_dataclass_attribute`` + catches the omission. Two-layer defence prevents the + drift class of bug round-4 introduced (and round-5 + flagged). + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult() + # Sanity: defaults are True for every flag the tuple + # knows about. + for name in prefetch.COMPLETION_FLAG_NAMES: + assert getattr(result, name) is True, ( + f"dataclass default for {name} must be True so " + f"_init_completion_flags has something to flip" + ) + + prefetch._init_completion_flags(result) + + # Post-preamble: every named flag is False. + for name in prefetch.COMPLETION_FLAG_NAMES: + assert getattr(result, name) is False, ( + f"{name} was not flipped to False by " + f"_init_completion_flags; the helper's loop is " + f"likely missing this attribute" + ) + + def test_completion_flag_names_covers_every_dataclass_attribute( + self, + ): + """Belt-and-braces guard against bidirectional drift: + + **Forward direction** (dataclass → tuple): someone adds a + new ``*_completed`` attribute to the dataclass but forgets + to add the name to ``COMPLETION_FLAG_NAMES``. Without + this check, the new attribute would silently default + ``True``, the preamble wouldn't reset it, and every + cross-work-type query would emit a misleading + authoritative-empty signal — the same bug round-3 fixed, + re-introduced one attribute at a time. + + **Reverse direction** (tuple → dataclass): someone + adds (or typos) an entry in ``COMPLETION_FLAG_NAMES`` + that doesn't correspond to a real dataclass attribute. + Empirically (round-6 critique): ``setattr`` accepts + arbitrary names silently, so a typo like + ``"ci_detail_complted"`` would NOT crash — it would + write a junk attribute on the instance while leaving + the REAL ``ci_detail_completed`` at the dataclass + default ``True``, silently re-introducing drift for + that section. The reverse-direction assertion below + catches this — set equality means neither direction + can drift. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult() + dataclass_flags = { + name + for name in dir(result) + if name.endswith("_completed") + and not name.startswith("_") + and isinstance(getattr(result, name), bool) + } + tuple_flags = set(prefetch.COMPLETION_FLAG_NAMES) + + missing_from_tuple = dataclass_flags - tuple_flags + assert not missing_from_tuple, ( + f"Dataclass exposes ``*_completed`` attributes the " + f"COMPLETION_FLAG_NAMES tuple does NOT enumerate: " + f"{sorted(missing_from_tuple)}. Add the names to " + f"the tuple in tools/_implementer_prefetch.py so the " + f"preamble resets them, otherwise cross-work-type " + f"drift returns one attribute at a time." + ) + + stale_in_tuple = tuple_flags - dataclass_flags + assert not stale_in_tuple, ( + f"COMPLETION_FLAG_NAMES contains entries that do NOT " + f"correspond to dataclass attributes (likely typos or " + f"stale renames): {sorted(stale_in_tuple)}. The " + f"preamble would silently write junk attributes onto " + f"the instance while leaving the REAL flags at the " + f"dataclass default ``True``, re-introducing the " + f"cross-work-type drift bug round-3 fixed." + ) + + def test_pr_fix_sentinel_excludes_authoritative_issue_signal( + self, sentinel, handoff_dir + ): + """End-to-end: a freshly-constructed pr_fix result fed + through the writer must produce a sentinel where + ``issue_body_completed`` is False — so when the worker + reads ``--field issue_body``, the three-case contract + emits empty stdout (fall through) rather than ``null\\n`` + (authoritative). + + Regression guard for the bug surfaced in the 2026-05-11 + critique where the dataclass-default-True flags leaked + across work-types. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult( + head_sha="aaa", + pr_details={"title": "T", "body": "B", + "head": {"ref": "h"}, "base": {"ref": "b"}}, + diff_text="diff", + ) + # Simulate what the fetcher's preamble would do. + prefetch._init_completion_flags(result) + + target = sentinel.write( + pr_number=30, work_type="pr_fix", + work_group="failing_ci_pr", result=result, + ) + payload = json.loads(target.read_text()) + + # Cross-work-type drift guard: + assert payload["issue_body_completed"] is False, ( + "pr_fix sentinel must mark issue_body as not-attempted " + "so worker reads '--field issue_body' as 'fall " + "through' rather than the misleading 'authoritative " + "empty body' signal" + ) + assert payload["issue_comments_completed"] is False + assert payload["request_changes_reviews_completed"] is False + + def test_new_issue_sentinel_excludes_pr_signals( + self, sentinel, handoff_dir + ): + """Mirror of the pr_fix test for ``new_issue`` work: + sentinel must mark every PR-shaped section + ``*_completed=False`` so workers reading those fields + don't see misleading authoritative-empty signals. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult( + issue_body="Implement refresh token endpoint.", + issue_comments=[{"id": 1, "body": "fyi"}], + ) + prefetch._init_completion_flags(result) + + target = sentinel.write( + pr_number=42, work_type="issue_impl", + work_group="new_issue", result=result, + ) + payload = json.loads(target.read_text()) + + # PR-shaped sections must all carry the not-attempted + # signal — no PR exists yet, so reading any of these + # fields should fall through to legacy GET (which will + # itself 404, exactly the signal the worker needs). + assert payload["pr_details_completed"] is False + assert payload["description_completed"] is False + assert payload["title_completed"] is False + assert payload["diff_completed"] is False + assert payload["ci_status_completed"] is False + assert payload["ci_detail_completed"] is False + assert payload["pr_comments_completed"] is False + assert payload["request_changes_reviews_completed"] is False + + +class TestFetcherPreambleBinding: + """Each public per-work-group fetcher MUST call + :func:`_init_completion_flags` before any early-exit. This is + the integration test the previous round was missing — the + helper-level tests verify the helper does its job, but only + these tests catch the regression where a future contributor + removes the helper call from ``_fetch_pr_context`` / + ``fetch_new_issue_context`` and silently re-introduces the + cross-work-type drift bug. + + Strategy: invoke the public fetcher with ``cfg.dry_run=True``, + which short-circuits AFTER the preamble runs but BEFORE any + Forgejo call. The returned result MUST have every + ``*_completed`` flag flipped to ``False`` — anything else + means the preamble didn't run. + + Without these tests, removing the preamble line from a + fetcher passes the full 1,146-test suite (empirically + verified during the 2026-05-11 round-3 critique). + """ + + @pytest.fixture + def dry_cfg(self, tmp_path): + """Dry-run dispatcher cfg via the shared conftest helper. + We deliberately do NOT use the heavier ``driver_chain`` + fixture — the contract under test is purely about the + dataclass state after preamble, no prompt rendering or + work-type dispatch required. + """ + return make_dispatch_config(tmp_path, dry_run=True) + + @staticmethod + def _all_completion_flags(result) -> dict[str, bool]: + """Snapshot every ``*_completed`` attribute on ``result``. + + Drives the enumeration off + :data:`_implementer_prefetch.COMPLETION_FLAG_NAMES` so a + future ``*_completed`` attribute added to the dataclass + is automatically covered. See + ``test_completion_flag_names_covers_every_dataclass_attribute`` + in ``TestNotAttemptedFlags`` for the belt-and-braces + guard that prevents the tuple from drifting from the + dataclass. + """ + prefetch = load_tool_module("_implementer_prefetch") + return { + name: getattr(result, name) + for name in prefetch.COMPLETION_FLAG_NAMES + } + + def test_fetch_pr_fix_context_calls_preamble(self, dry_cfg): + """``fetch_pr_fix_context`` MUST initialise every + ``*_completed`` flag to ``False`` before returning. Without + this, ``pr_fix`` cycles would carry the dataclass default + ``True`` for ``issue_body`` / ``issue_comments`` / + ``request_changes_reviews`` and the worker would + misinterpret empty values as 'fetched and confirmed empty' + rather than 'fall through to legacy GET'. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.fetch_pr_fix_context( + dry_cfg, {"number": 30, "title": "Fix login redirect"} + ) + flags = self._all_completion_flags(result) + # Pin every flag — anything that comes back True means + # the preamble didn't run for that flag. + for name, value in flags.items(): + assert value is False, ( + f"{name} was {value!r} after fetch_pr_fix_context; " + f"expected False. The fetcher's preamble call to " + f"_init_completion_flags is missing or partial." + ) + + def test_fetch_request_changes_context_calls_preamble( + self, dry_cfg + ): + """``fetch_request_changes_context`` shares + ``_fetch_pr_context`` with ``fetch_pr_fix_context``, but + the binding must be verified independently — a future + refactor could split them and only one fetcher would + retain the preamble. Regression guard.""" + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.fetch_request_changes_context( + dry_cfg, {"number": 31, "title": "Address review feedback"} + ) + flags = self._all_completion_flags(result) + for name, value in flags.items(): + assert value is False, ( + f"{name} was {value!r} after " + f"fetch_request_changes_context; expected False." + ) + + def test_fetch_new_issue_context_calls_preamble(self, dry_cfg): + """``fetch_new_issue_context`` has its own preamble call + (separate from ``_fetch_pr_context``). Independent + regression guard — removing the line from the new_issue + fetcher specifically would re-introduce drift for issue + cycles even if the PR fetchers still pass.""" + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.fetch_new_issue_context( + dry_cfg, {"number": 42, "title": "Add JWT refresh"} + ) + flags = self._all_completion_flags(result) + for name, value in flags.items(): + assert value is False, ( + f"{name} was {value!r} after " + f"fetch_new_issue_context; expected False." + ) + + def test_preamble_runs_before_dryrun_gate_on_non_dryrun_path( + self, tmp_path, monkeypatch + ): + """Stronger regression guard: defends against the + ``preamble-moved-inside-dry_run-block`` pattern. + + Each ``fetch_*_context`` function MUST call + ``_init_completion_flags`` BEFORE the ``if cfg.dry_run:`` + gate, so the preamble runs on the production + (non-dry-run) path too. The other three binding tests + only exercise the dry-run early-exit, so a regression + that placed the preamble inside the dry-run block would + pass them all while silently breaking production cycles. + + Strategy: stub ``_review_fetch.fetch_pr_details`` to + return ``None`` so the fetcher early-exits at the first + Forgejo call (before any other helper runs). The result + MUST still carry ``pr_details_completed=False`` (set by + the preamble; the early-exit branch comments "already + False"). If the preamble lives behind the dry-run gate, + this flag would be the dataclass default ``True`` and + the assertion fails — exactly the regression catch. + """ + cfg_obj = make_dispatch_config(tmp_path, dry_run=False) + prefetch = load_tool_module("_implementer_prefetch") + # Patch the fetcher module's bound ``_review_fetch`` so + # the early-exit branch fires deterministically without + # hitting real Forgejo. + monkeypatch.setattr( + prefetch._review_fetch, + "fetch_pr_details", + lambda _cfg, _n: None, + ) + result = prefetch.fetch_pr_fix_context( + cfg_obj, {"number": 30, "title": "Fix login redirect"} + ) + # Early-exit hit; pr_details_completed MUST be False + # (set by preamble — NOT the dataclass default True). + assert result.pr_details_completed is False, ( + "pr_details_completed was True after non-dry-run " + "early-exit; the preamble is likely guarded by " + "``if cfg.dry_run:`` instead of running before the " + "gate. The other binding tests only see the " + "dry-run path and would miss this." + ) + # The remaining flags must also stay False — the + # early-exit returns before the success-flip pattern + # touches any of them, and the preamble flipped each one + # to False on entry. + flags = self._all_completion_flags(result) + for name, value in flags.items(): + assert value is False, ( + f"{name} was {value!r} after early-exit on " + f"non-dry-run path; preamble likely didn't run." + ) + # Sanity: the early-exit also tags ``data_complete`` and + # an error_kind so an operator triaging from telemetry + # sees the fetch failed (rather than "all green, just + # empty"). + assert result.data_complete is False + assert any( + "pr_details" in k for k in result.error_kinds + ), f"expected pr_details error_kind; got {result.error_kinds}" + + +def test_completion_flags_round_trip_failure_states( + sentinel, handoff_dir +): + """When the dispatcher's fetch fails for a section, the + corresponding ``*_completed`` flag must flow through the + writer to the on-disk sentinel intact. The worker-side reader + uses these flags to decide whether to fall through to the + legacy GET — a missing or dropped flag would cause the worker + to serve stale empty data as authoritative. + + Uses the real :class:`ImplementerPrefetchResult` so a future + rename of a ``*_completed`` field on the dataclass fails this + test rather than silently dropping the flag. + """ + prefetch = load_tool_module("_implementer_prefetch") + result = prefetch.ImplementerPrefetchResult( + head_sha="", + pr_details=None, + pr_details_completed=False, + diff_text="", + diff_unavailable=True, + diff_completed=False, + ci_status=None, + ci_status_completed=False, + ci_detail=[], + ci_detail_completed=False, + pr_comments=[], + pr_comments_completed=False, + request_changes_reviews=[], + request_changes_reviews_completed=False, + issue_body="", + issue_body_completed=False, + issue_comments=[], + issue_comments_completed=False, + linked_issues=[], + linked_issues_completed=False, + epic_issue=None, + epic_completed=False, + data_complete=False, + error_kinds=["pr_details:fetch-failed"], + ) + + target = sentinel.write( + pr_number=88, work_type="pr_fix", + work_group="failing_ci_pr", result=result, + ) + payload = json.loads(target.read_text()) + # All completion flags must round-trip as False — the worker + # checks each one and falls through to legacy GET for any + # field whose flag is False. + failure_flags = {k: v for k, v in payload.items() + if k.endswith("_completed")} + assert all(v is False for v in failure_flags.values()), ( + f"expected ALL *_completed flags to be False on totally-" + f"failed prefetch; got {failure_flags}" + ) + assert payload["data_complete"] is False + + def test_write_atomic_no_partial_file_on_serialisation_error( sentinel, handoff_dir, monkeypatch ): @@ -357,3 +926,15 @@ def test_real_prefetch_result_round_trip(sentinel, handoff_dir): assert payload["data_complete"] is True assert payload["error_kinds"] == [] assert payload["listing_title"] == "Listing title" + # Completion flags introduced 2026-05-11 — every flag must + # round-trip True for a fully-successful prefetch. + assert payload["description_completed"] is True + assert payload["title_completed"] is True + assert payload["diff_completed"] is True + assert payload["issue_body_completed"] is True + assert payload["ci_status_completed"] is True + assert payload["ci_detail_completed"] is True + assert payload["pr_comments_completed"] is True + assert payload["request_changes_reviews_completed"] is True + assert payload["linked_issues_completed"] is True + assert payload["epic_completed"] is True diff --git a/tools/_implementer_prefetch.py b/tools/_implementer_prefetch.py index 4918986ca..768e9e549 100644 --- a/tools/_implementer_prefetch.py +++ b/tools/_implementer_prefetch.py @@ -100,6 +100,49 @@ DEFAULT_COMMENT_MAX_CHARS = 4000 DEFAULT_CI_LOG_TAIL_CHARS = 4000 +# Single source of truth for the dataclass's per-section completion +# flag attributes. Consumed by: +# +# - :func:`_init_completion_flags` (loops over the tuple to reset +# every flag rather than enumerating 10 explicit assignments). +# - The sentinel writer +# :mod:`_pr_context_sentinel.\_to_dict` (re-exports the +# tuple at module scope and loops to overlay every flag onto +# the projected dict; see that module's import block). +# - The worker-side reader +# :func:`tools.implementer_pr_context._project_field` (the +# ``metadata`` branch enumerates ``*_completed`` keys present +# in the payload — naturally tracking writer-side additions +# without sharing the tuple directly so the reader stays a +# stand-alone script with no dispatcher imports). +# - The fetcher-binding regression tests in +# ``tests/auto_agents/test_pr_context_sentinel.py`` (import this +# tuple to enumerate every flag a fetcher's preamble must reset +# — including any flag added in the future). +# +# Adding a new ``*_completed`` flag to +# :class:`ImplementerPrefetchResult` MUST also append the +# attribute name to this tuple — the binding tests will fail +# loudly if the new flag isn't initialised by the preamble. +# Conversely, a typo'd / stale entry in the tuple is caught by +# the belt-and-braces guard +# ``test_completion_flag_names_covers_every_dataclass_attribute`` +# which asserts the tuple set equals the dataclass set +# (bidirectional drift check). +COMPLETION_FLAG_NAMES: tuple[str, ...] = ( + "pr_details_completed", + "diff_completed", + "ci_status_completed", + "ci_detail_completed", + "pr_comments_completed", + "request_changes_reviews_completed", + "issue_body_completed", + "issue_comments_completed", + "linked_issues_completed", + "epic_completed", +) + + # ─── Result dataclass ─────────────────────────────────────────────────────── @@ -137,10 +180,22 @@ class ImplementerPrefetchResult: head_sha: str = "" pr_details: dict[str, Any] | None = None + # True iff ``pr_details`` was fetched successfully. The + # ``description`` and ``title`` fields are derived from + # ``pr_details``; a legitimately-empty PR description with this + # flag True maps to "authoritative empty" (worker skips legacy + # GET) whereas an empty description with this flag False + # (transient fetch failure) maps to "fall through" via the + # worker-side three-case contract. + pr_details_completed: bool = True diff_text: str = "" diff_truncated: bool = False diff_unavailable: bool = False diff_info: dict[str, int] = field(default_factory=dict) + # True iff the diff was fetched successfully (truncation does + # not flip this — a truncated diff is still authoritative). + # Derives ``diff_completed`` in the sentinel projection. + diff_completed: bool = True ci_status: dict[str, Any] | None = None ci_status_completed: bool = True ci_detail: list[dict[str, Any]] = field(default_factory=list) @@ -150,6 +205,12 @@ class ImplementerPrefetchResult: request_changes_reviews: list[dict[str, Any]] = field(default_factory=list) request_changes_reviews_completed: bool = True issue_body: str = "" + # True iff the issue body was fetched successfully (only + # meaningful for ``new_issue`` work; the field stays empty for + # ``pr_fix`` / ``request_changes_pr`` where the issue is not + # part of the work shape but the flag is still True because + # "nothing was attempted" is itself a complete answer). + issue_body_completed: bool = True issue_comments: list[dict[str, Any]] = field(default_factory=list) issue_comments_completed: bool = True linked_issues: list[dict[str, Any]] = field(default_factory=list) @@ -241,6 +302,14 @@ def fetch_pr_fix_context( Does NOT fetch active reviews — the failing-CI bucket is review-state-agnostic. Use :func:`fetch_request_changes_context` for the bucket where reviews are load-bearing. + + Sections this work_type does NOT attempt (``issue_body``, + ``issue_comments``, ``request_changes_reviews``) carry + ``*_completed=False`` in the result so the worker-side + three-case contract correctly signals "not attempted; fall + through to legacy GET" instead of "fetched and confirmed + empty" for those fields. See :func:`_init_completion_flags` + for the preamble + success-flip pattern. """ return _fetch_pr_context(cfg, item, include_active_reviews=False) @@ -255,10 +324,57 @@ def fetch_request_changes_context( inline comments. The worker uses these to drive its fix loop — every blocking concern must be addressed before the worker pushes. + + Sections this work_type does NOT attempt (``issue_body``, + ``issue_comments``) carry ``*_completed=False`` in the result + — same rationale as :func:`fetch_pr_fix_context`. """ return _fetch_pr_context(cfg, item, include_active_reviews=True) +def _init_completion_flags(result: ImplementerPrefetchResult) -> None: + """Reset every ``*_completed`` flag on ``result`` to ``False`` + before the fetcher attempts any Forgejo round-trip. + + This is the preamble half of the "preamble + success-flip" + pattern used by every per-work-group fetcher. After this runs: + + - Sections the work_type **never attempts** (e.g. ``issue_body`` + for ``pr_fix``) stay ``False`` through to the on-disk + sentinel. The worker reading ``--field issue_body`` then + gets empty stdout and falls through to its legacy GET — + the correct behaviour for "dispatcher didn't try; you go + ask Forgejo yourself". + + - Sections the work_type **does attempt** (e.g. ``pr_details`` + for ``pr_fix``) start ``False`` and the fetcher's success + path flips them ``True`` explicitly. This lets early-exits + (e.g. ``pr_details`` 5xx → return before diff / CI / comments + / reviews) leave the not-yet-attempted flags ``False`` so + the worker correctly falls through for those fields rather + than being told a stale ``[]`` / ``null`` is authoritative. + + Why ONE helper for both ``_fetch_pr_context`` and + ``fetch_new_issue_context``: the two work-shape families + differ in WHICH flags get flipped ``True`` on success, but + they don't differ in the preamble — every flag starts + ``False``. Per-work-type intent is documented on the + individual fetcher docstrings, not duplicated across two + identical helper bodies. + + Loops over :data:`COMPLETION_FLAG_NAMES` rather than + enumerating attributes explicitly so adding a new + ``*_completed`` flag to the dataclass automatically extends + the preamble — provided the contributor also adds the name + to the tuple. The fetcher-binding regression tests share the + same tuple, so any drift between the dataclass and the + preamble fails a test rather than silently shipping a + half-reset result. + """ + for name in COMPLETION_FLAG_NAMES: + setattr(result, name, False) + + def _fetch_pr_context( cfg: Any, item: dict[str, Any], @@ -278,6 +394,19 @@ def _fetch_pr_context( post_session_action can short-circuit on partial context. """ result = ImplementerPrefetchResult() + # Preamble: every ``*_completed`` flag starts ``False`` BEFORE + # any potential early-exit (invalid PR number, dry-run, fetch + # failure). The flag drives the worker-side three-case + # contract: False → empty stdout → worker falls through to + # legacy GET; True → either ``[]\n`` / ``null\n`` (authoritative + # empty) or content. See :func:`_init_completion_flags`. + # + # ``pr_fix`` work NEVER attempts ``issue_body`` / + # ``issue_comments`` / ``request_changes_reviews``; those + # flags STAY False through to the sentinel. + # ``request_changes_pr`` additionally attempts + # ``request_changes_reviews`` and flips its flag below. + _init_completion_flags(result) if cfg.dry_run: return result pr_number = int(item.get("number") or 0) @@ -291,9 +420,14 @@ def _fetch_pr_context( pr_details = _review_fetch.fetch_pr_details(cfg, pr_number) result.pr_details = pr_details if pr_details is None: + # ``pr_details_completed`` is already False (set by the + # preamble); leave it False. Any downstream flag (diff, + # ci_*, pr_comments) also stays False because we early- + # exit before reaching those fetches. result.data_complete = False result.error_kinds.append("pr_details:fetch-failed") return result + result.pr_details_completed = True head = pr_details.get("head") if isinstance(pr_details.get("head"), dict) else {} result.head_sha = str(head.get("sha") or item.get("head_sha") or "") @@ -306,26 +440,41 @@ def _fetch_pr_context( result.diff_info = diff_info if not diff_text: result.diff_unavailable = True + # ``diff_completed`` is already False (preamble); leave it. result.data_complete = False result.error_kinds.append( f"diff:{diff_error or 'no-diff-returned'}" ) - elif diff_truncated: - result.data_complete = False + else: + # Truncation does NOT flip ``diff_completed`` — a truncated + # diff is still authoritative (just hard-capped). It DOES + # flip ``data_complete`` so the cycle archive flags partial + # context. + result.diff_completed = True + if diff_truncated: + result.data_complete = False # CI combined status ci_status = _review_fetch.fetch_ci_status(cfg, result.head_sha) result.ci_status = ci_status if ci_status is None: - result.ci_status_completed = False + # ``ci_status_completed`` is already False (preamble). result.data_complete = False result.error_kinds.append("ci_status:fetch-failed") + else: + result.ci_status_completed = True # Per-check detail (only when overall != success — the per-check # endpoint is verbose and would burn quota for green PRs). if isinstance(ci_status, dict): overall_state = ci_status.get("state") - if overall_state and overall_state != "success": + if overall_state == "success": + # Green PR: no failing-check detail to fetch. The empty + # ``ci_detail`` list IS authoritative; flip the flag + # True so the worker reads "0 failing checks" rather + # than "fetch failed; retry". + result.ci_detail_completed = True + elif overall_state: ci_detail, ci_detail_completed = _review_fetch.fetch_ci_check_detail( cfg, result.head_sha ) @@ -400,8 +549,29 @@ def fetch_new_issue_context( - The Epic body when the issue references one. head_sha stays empty — the worker will create the branch itself. + + Sections this work_type does NOT attempt (``pr_details`` and + everything derived from it: description, title, diff, CI, + pr_comments, request_changes_reviews) carry ``*_completed=False`` + in the result so the worker-side three-case contract correctly + signals "not attempted; fall through to legacy GET" instead of + "fetched and confirmed empty" for those fields. See + :func:`_init_completion_flags` for the preamble + success-flip + pattern. """ result = ImplementerPrefetchResult() + # Preamble: every ``*_completed`` flag starts ``False`` BEFORE + # any potential early-exit (invalid issue number, dry-run, + # fetch failure). See :func:`_init_completion_flags`. + # + # ``new_issue`` work NEVER attempts PR-shaped sections + # (``pr_details`` and everything derived from it: ``title``, + # ``description``, ``diff``, ``ci_*``, ``pr_comments``, + # ``request_changes_reviews``); those flags STAY False + # through to the sentinel. The fetcher below flips + # ``issue_body`` / ``issue_comments`` / ``linked_issues`` / + # ``epic`` to True on successful fetch. + _init_completion_flags(result) if cfg.dry_run: return result issue_number = int(item.get("number") or 0) @@ -412,6 +582,7 @@ def fetch_new_issue_context( issue, issue_completed = _fetch_issue(cfg, issue_number) if issue is None: + # ``issue_body_completed`` is already False (preamble). result.data_complete = False result.error_kinds.append( "issue:fetch-failed" if not issue_completed else "issue:not-found" @@ -419,6 +590,9 @@ def fetch_new_issue_context( return result body = issue.get("body") if isinstance(issue, dict) else None result.issue_body = str(body or "") + # Successful fetch — even if ``body`` is empty, the empty value + # is authoritative. + result.issue_body_completed = True # Issue comments — same paginated path as PR issue-style comments # because Forgejo treats issues and PRs as one conversation @@ -464,7 +638,13 @@ def _resolve_links_and_epic( cfg, body_for_links, max_issues=DEFAULT_MAX_LINKED_ISSUES ) epic_number = parse_epic_reference(body_for_links) - if epic_number is not None: + if epic_number is None: + # No Epic referenced at all — authoritative "no Epic" + # answer. Flip the flag True so the worker's + # ``--field epic`` read emits ``null\n`` (authoritative) + # rather than empty stdout (fall through). + result.epic_completed = True + else: # The linked-issue resolver may already have fetched the same # number via a `Closes #N` reference. Rather than refetching, # promote it from the linked list to the epic slot. @@ -478,11 +658,12 @@ def _resolve_links_and_epic( if promoted is not None: result.epic_issue = promoted linked_issues = remaining + result.epic_completed = True else: epic_body, epic_completed = _fetch_issue(cfg, epic_number) result.epic_issue = epic_body if not epic_completed: - result.epic_completed = False + # Already False from preamble; leave it. result.data_complete = False result.error_kinds.append("epic:fetch-failed") elif epic_body is None: @@ -497,6 +678,9 @@ def _resolve_links_and_epic( # "Epic body fetched fine". result.epic_completed = True result.error_kinds.append("epic:not-found") + else: + # Epic body fetched successfully. + result.epic_completed = True result.linked_issues = linked_issues result.linked_issues_completed = linked_completed if not linked_completed: @@ -505,6 +689,7 @@ def _resolve_links_and_epic( __all__ = ( + "COMPLETION_FLAG_NAMES", "DEFAULT_BODY_MAX_CHARS", "DEFAULT_CI_LOG_TAIL_CHARS", "DEFAULT_COMMENT_MAX_CHARS", diff --git a/tools/_pr_context_sentinel.py b/tools/_pr_context_sentinel.py index 03d0371eb..265157202 100644 --- a/tools/_pr_context_sentinel.py +++ b/tools/_pr_context_sentinel.py @@ -51,9 +51,28 @@ import datetime as _dt import json import logging import os +import sys from pathlib import Path from typing import Any +_TOOLS_DIR = str(Path(__file__).resolve().parent) +if _TOOLS_DIR not in sys.path: + sys.path.insert(0, _TOOLS_DIR) +from _loader import ( # noqa: E402 type: ignore[import-not-found] + load_sibling as _load_sibling, +) + +# Single source of truth for the per-section completion flag names. +# Defined in :mod:`_implementer_prefetch`; imported here so the +# sentinel writer's projection logic loops over the same tuple +# the fetcher's preamble loops over. A future contributor adding a +# new ``*_completed`` flag to the dataclass updates the tuple +# once — the preamble (``_init_completion_flags``), the writer +# (this module's ``_to_dict``), and the binding regression tests +# all pick it up automatically. +_prefetch = _load_sibling("_implementer_prefetch", "_implementer_prefetch.py") +COMPLETION_FLAG_NAMES: tuple[str, ...] = _prefetch.COMPLETION_FLAG_NAMES + _logger = logging.getLogger("pr_context_sentinel") @@ -109,6 +128,35 @@ def _truncate_for_sentinel(value: str, max_chars: int = 200_000) -> str: return value[:max_chars] + f"\n[…truncated {dropped} bytes from sentinel]" +_COMPLETION_DEFAULT_TRUE = True + + +def _read_flag(result: Any, name: str) -> bool: + """Read a "good-when-True" status flag off ``result``. + + Applies to ``*_completed`` flags AND ``data_complete`` — + every boolean on :class:`ImplementerPrefetchResult` whose + semantics are "True means the fetch succeeded / aggregate + is healthy". NOT for inverted flags like ``diff_truncated`` + or ``diff_unavailable`` (True = bad state), which still use + raw ``bool(getattr(...))`` because the strict-True default + would mask their failure semantics. + + Strict-True semantics: only returns False when the attribute + is explicitly ``False``. A missing attribute or ``None`` + defaults to True. This is more conservative than the previous + ``bool(getattr(..., True))`` which would silently flip None → + False — making a typo'd attribute name (e.g. ``result.epic_completed + = None``) silently flip the flag and serve "fetched failed" + semantics to the worker. + + Tests that need to forcibly flip a flag False MUST set it to + the literal ``False``, not to a falsy proxy. + """ + val = getattr(result, name, _COMPLETION_DEFAULT_TRUE) + return val is not False + + def _to_dict(result: Any) -> dict[str, Any]: """Project an ``ImplementerPrefetchResult`` into a plain dict suitable for JSON serialisation. @@ -119,56 +167,102 @@ def _to_dict(result: Any) -> dict[str, Any]: reference), the projection records ``null`` so the worker-side reader can distinguish "section was attempted, came back empty" from "section was never attempted". + + Completion flags are NOT spelled out per key in the projected + dict — the function builds the value-projection base then + overlays a ``*_completed`` entry for every name in + :data:`COMPLETION_FLAG_NAMES`. That keeps the dataclass, the + preamble (``_init_completion_flags``), the writer (this + function), and the regression tests on a single source of + truth: adding a new flag to the tuple automatically + propagates here. """ pr_details = getattr(result, "pr_details", None) or {} head = pr_details.get("head") or {} base = pr_details.get("base") or {} - return { + # ─── pr_details-derivation invariant ──────────────────────── + # ``description`` and ``title`` are extracted from the single + # ``pr_details`` payload, so their completion semantics MUST + # collapse into the underlying ``pr_details_completed`` flag. + # Three sentinel keys (``pr_details_completed``, + # ``title_completed``, ``description_completed``) project + # the same source flag — a deliberate redundancy that makes + # ``--field metadata`` self-documenting (operators don't have + # to know about the derivation) at the cost of one extra + # boolean each in the serialised payload. + # + # The base ``pr_details_completed`` is overlaid below by the + # ``COMPLETION_FLAG_NAMES`` loop; ``title_completed`` and + # ``description_completed`` are explicit derived keys here + # because they are NOT in the dataclass (and therefore not + # in the canonical tuple). + # + # Invariant: a future change that fetches title or description + # from a SEPARATE endpoint (so they can succeed / fail + # independently of pr_details) MUST break this collapse — + # introduce dedicated ``title_completed`` / + # ``description_completed`` fields on + # :class:`_implementer_prefetch.ImplementerPrefetchResult`, + # add them to ``COMPLETION_FLAG_NAMES``, and drop the + # derived keys below. + pr_details_completed = _read_flag(result, "pr_details_completed") + projected: dict[str, Any] = { "title": pr_details.get("title") or "", + "title_completed": pr_details_completed, "description": _truncate_for_sentinel(pr_details.get("body") or ""), + "description_completed": pr_details_completed, "head_sha": getattr(result, "head_sha", "") or "", "head_ref": head.get("ref") or "", "base_ref": base.get("ref") or "", "ci_status": getattr(result, "ci_status", None), - "ci_status_completed": bool( - getattr(result, "ci_status_completed", True) - ), "ci_detail": list(getattr(result, "ci_detail", []) or []), - "ci_detail_completed": bool( - getattr(result, "ci_detail_completed", True) - ), "pr_comments": list(getattr(result, "pr_comments", []) or []), - "pr_comments_completed": bool( - getattr(result, "pr_comments_completed", True) - ), "request_changes_reviews": list( getattr(result, "request_changes_reviews", []) or [] ), - "request_changes_reviews_completed": bool( - getattr(result, "request_changes_reviews_completed", True) - ), "issue_body": _truncate_for_sentinel( getattr(result, "issue_body", "") or "" ), "issue_comments": list(getattr(result, "issue_comments", []) or []), - "issue_comments_completed": bool( - getattr(result, "issue_comments_completed", True) - ), "linked_issues": list(getattr(result, "linked_issues", []) or []), - "linked_issues_completed": bool( - getattr(result, "linked_issues_completed", True) - ), "epic": getattr(result, "epic_issue", None), - "epic_completed": bool(getattr(result, "epic_completed", True)), "diff": _truncate_for_sentinel( getattr(result, "diff_text", "") or "" ), "diff_truncated": bool(getattr(result, "diff_truncated", False)), "diff_unavailable": bool(getattr(result, "diff_unavailable", False)), "diff_info": dict(getattr(result, "diff_info", {}) or {}), - "data_complete": bool(getattr(result, "data_complete", True)), + "data_complete": _read_flag(result, "data_complete"), "error_kinds": list(getattr(result, "error_kinds", []) or []), } + # Overlay every dataclass-side ``*_completed`` flag via the + # canonical tuple. A new flag added to ``COMPLETION_FLAG_NAMES`` + # automatically appears in the sentinel without further + # changes here. Uses the strict-True ``_read_flag`` defensive + # so a typo'd attribute (or accidental ``None`` assignment) + # defaults to True (the safer fallback for the worker) rather + # than silently flipping to "fetch failed". + # + # The ``not in projected`` guard is a belt-and-braces defence + # against accidental schema-base collisions: if a future + # contributor adds (say) ``"epic_completed": ...`` to the + # value-projection base above and forgets to remove the + # corresponding entry from ``COMPLETION_FLAG_NAMES`` (or + # vice-versa), the overlay would silently clobber the base + # value. The assert fails loudly with the offending name so + # the contributor sees the conflict immediately. The two + # legitimately derived keys (``title_completed`` / + # ``description_completed``) are NOT in the canonical tuple + # so they bypass this loop entirely. + for name in COMPLETION_FLAG_NAMES: + assert name not in projected, ( + f"_to_dict schema-base already contains '{name}' — " + f"either remove the explicit key from the value-projection " + f"base or remove the name from COMPLETION_FLAG_NAMES. The " + f"overlay loop would silently clobber the base value." + ) + projected[name] = _read_flag(result, name) + return projected def write( diff --git a/tools/dispatch_implementer.py b/tools/dispatch_implementer.py index 955834b2d..d52c71840 100644 --- a/tools/dispatch_implementer.py +++ b/tools/dispatch_implementer.py @@ -307,19 +307,29 @@ def _prefetch_prompt( # agents' summarisation, or (b) re-issuing the Forgejo curls. # Best-effort: a write failure logs WARNING and the worker falls # back to the prompt content (or to curl). - try: - _pr_context_sentinel.write( - pr_number=pr_number, - work_type=_work_type_for_group(group.name), - work_group=group.name, - result=result, - item=item, - ) - except Exception as e: # noqa: BLE001 — best-effort - _logger.warning( - "PR context sentinel write failed for PR #%s: %s", - pr_number, e, - ) + # + # Dry-run gate: dry-run is the operator-visible "preview, no I/O" + # contract. The fetchers themselves already short-circuit in + # dry-run and return empty results — writing a sentinel for an + # empty result would (a) violate the no-I/O invariant and (b) + # leave a stale empty-shaped sentinel on disk that the next real + # cycle would have to overwrite. The workspace handoff is + # gated upstream in ``_build_clone_section``; this is the matching + # gate for the PR-context handoff. + if not cfg.dry_run: + try: + _pr_context_sentinel.write( + pr_number=pr_number, + work_type=_work_type_for_group(group.name), + work_group=group.name, + result=result, + item=item, + ) + except Exception as e: # noqa: BLE001 — best-effort + _logger.warning( + "PR context sentinel write failed for PR #%s: %s", + pr_number, e, + ) return text diff --git a/tools/implementer_pr_context.py b/tools/implementer_pr_context.py index cc58f253e..6dd16630d 100755 --- a/tools/implementer_pr_context.py +++ b/tools/implementer_pr_context.py @@ -169,42 +169,104 @@ def _load(pr_number: int) -> tuple[dict[str, Any] | None, str | None]: _MISSING: Any = object() +def _gated_list( + payload: dict[str, Any], value_key: str, completed_key: str +) -> Any: + """Three-case helper for list-valued fields. ``_MISSING`` when + ``completed_key`` is False; the value (or ``[]`` if absent / + falsy) otherwise. Empty list serialises as ``[]\\n``, which + the worker reads as authoritative-empty (skip legacy GET).""" + if not payload.get(completed_key, True): + return _MISSING + return payload.get(value_key) or [] + + +def _gated_text( + payload: dict[str, Any], value_key: str, completed_key: str +) -> Any: + """Three-case helper for plain-text fields. + + - completion flag False → ``_MISSING`` (worker falls through to + legacy GET). + - completion flag True + empty body → ``None`` (rendered as + ``null\\n``; worker reads as authoritative-empty and skips + the legacy GET). + - completion flag True + non-empty body → the body verbatim. + + The completion-flag-aware path is what lets a legitimately-empty + PR description (``description_completed=True``, ``description=""``) + cleanly signal "no description, don't re-fetch" rather than + forcing a redundant GET. Before the flag existed, every empty + description / title / diff / issue_body cost the worker a + redundant Forgejo round-trip. + """ + if not payload.get(completed_key, True): + return _MISSING + body = payload.get(value_key) + if not body: + return None + return body + + def _project_field(payload: dict[str, Any], field: str) -> Any: """Return the slice of ``payload`` selected by ``--field``. - Returns :data:`_MISSING` when the field is genuinely absent or - when the dispatcher's fetch did not complete successfully (per - the field's ``*_completed`` flag). Returns ``None`` when the - fetch succeeded and the dispatcher confirmed absence (e.g. no - Epic reference). Returns the value otherwise. + Implements the **three-case contract** the worker relies on: - The two-case ``_MISSING`` vs ``None`` distinction is what stops - the worker from re-curling Forgejo for sections the dispatcher - has already confirmed are empty. + - :data:`_MISSING` — the field is genuinely absent OR the + dispatcher's fetch for it did not complete successfully (per + the field's ``*_completed`` flag). :func:`_emit` renders this + as empty stdout; the worker falls through to its legacy GET. + - ``None`` — the dispatcher's fetch succeeded AND confirmed + the field is empty in a way the JSON schema can encode (e.g. + ``epic=None`` means "PR has no parent Epic"). :func:`_emit` + renders this as ``null\\n``; the worker skips the legacy GET. + - any other value — the value itself, rendered verbatim (plain + text for ``description``/``title``/``diff``/``issue_body``, + pretty-printed JSON for the rest). Empty lists / empty dicts + ALSO fall through this case and render as ``[]\\n`` / ``{}\\n`` + — they carry the "fetched, confirmed empty" semantics for + list/dict-valued fields. + + The :data:`_MISSING` vs ``None`` vs value distinction is what + stops the worker from re-curling Forgejo for sections the + dispatcher has already confirmed are empty. Mapping all three + cases to ``None`` (the pre-2026-05-10 behavior) collapsed the + "didn't try" and "tried, confirmed empty" cases together and + every confirmed-empty section cost the worker a redundant GET. """ if field == "all": return payload if field == "description": - # PR body. Treat empty string as "no description; fall - # through" because an empty description from a successful - # fetch is functionally indistinguishable from a no-fetch - # for the worker's purposes — and there's no - # ``description_completed`` flag to disambiguate. - body = payload.get("description") - if not body: - return _MISSING - return body + return _gated_text(payload, "description", "description_completed") if field == "title": - title = payload.get("title") - if not title: - return _MISSING - return title + return _gated_text(payload, "title", "title_completed") if field == "diff": - diff = payload.get("diff") - if not diff: - return _MISSING - return diff + return _gated_text(payload, "diff", "diff_completed") + if field == "issue_body": + return _gated_text(payload, "issue_body", "issue_body_completed") if field == "metadata": + # The ``completion`` sub-object surfaces the per-section + # ``*_completed`` flags so an operator inspecting via + # ``--field metadata`` can see ALL fetcher state in one + # query without having to iterate every ``--field``. The + # worker's per-field reads still rely on the individual + # field projections — this is for human/operator triage. + # + # Enumerates payload keys (rather than hardcoding the + # name list) so a new ``*_completed`` flag added to the + # writer's ``COMPLETION_FLAG_NAMES`` tuple appears here + # automatically — keeps this reader script standalone + # (no dispatcher / prefetch imports) while still tracking + # writer-side additions without code changes. Stable + # sort order so two consecutive ``--field metadata`` + # reads produce byte-identical output (worker / operator + # diff tooling depends on this). + completion = { + k.removesuffix("_completed"): payload.get(k, True) + for k in sorted(payload) + if k.endswith("_completed") + } return { "schema_version": payload.get("schema_version"), "pr_number": payload.get("pr_number"), @@ -215,17 +277,39 @@ def _project_field(payload: dict[str, Any], field: str) -> Any: "base_ref": payload.get("base_ref"), "listing_title": payload.get("listing_title"), "data_complete": payload.get("data_complete"), + "completion": completion, "error_kinds": payload.get("error_kinds") or [], "created_at": payload.get("created_at"), "dispatcher_pid": payload.get("dispatcher_pid"), } if field == "ci": - # Per-check completion is decided by ``ci_status_completed`` / - # ``ci_detail_completed`` — the worker reads those alongside - # the bundle and decides whether to fall through. We always - # emit the bundle when the sentinel exists at all, even if - # the inner status is null, because the completion booleans - # are themselves authoritative data the worker needs. + # ─── Contract divergence (intentional) ──────────────── + # Every OTHER field gated on a completion flag uses + # _gated_list / _gated_text and returns ``_MISSING`` + # when the flag is False (empty stdout → worker falls + # through to legacy GET). ``ci`` is the exception: it + # ALWAYS emits the bundle when the sentinel exists at + # all, and the worker is expected to inspect the inner + # ``status_completed`` / ``detail_completed`` booleans + # to decide what to do. + # + # Why diverge: + # 1. CI carries TWO independent completion signals + # (status + detail). Collapsing them into one + # ``_MISSING`` would lose the case where the status + # summary fetch succeeded but the per-check detail + # fetch failed — the worker still has useful + # aggregate state ("PR is red"), it just lacks + # per-check evidence. + # 2. A green PR legitimately has ``ci_detail=[]`` (no + # failing checks to enumerate) with + # ``ci_detail_completed=True``. Emitting that as a + # bundle is the right answer. + # + # Workers reading ``--field ci`` MUST check both + # completion booleans before treating the bundle as + # authoritative. See SKILL.md "ci field exception" for + # the worker-side procedure. return { "status": payload.get("ci_status"), "checks": payload.get("ci_detail") or [], @@ -240,20 +324,18 @@ def _project_field(payload: dict[str, Any], field: str) -> Any: # cycle that also happened to have a stale PR), the previous # ``or`` chain would return the wrong one. if payload.get("work_type") == "issue_impl": - if not payload.get("issue_comments_completed", True): - return _MISSING - return payload.get("issue_comments") or [] - if not payload.get("pr_comments_completed", True): - return _MISSING - return payload.get("pr_comments") or [] + return _gated_list( + payload, "issue_comments", "issue_comments_completed" + ) + return _gated_list(payload, "pr_comments", "pr_comments_completed") if field == "reviews": - if not payload.get("request_changes_reviews_completed", True): - return _MISSING - return payload.get("request_changes_reviews") or [] + return _gated_list( + payload, + "request_changes_reviews", + "request_changes_reviews_completed", + ) if field == "issues": - if not payload.get("linked_issues_completed", True): - return _MISSING - return payload.get("linked_issues") or [] + return _gated_list(payload, "linked_issues", "linked_issues_completed") if field == "epic": # ``epic_completed=False`` means the fetch failed; fall # through. ``epic_completed=True`` with ``epic=None`` means @@ -262,14 +344,6 @@ def _project_field(payload: dict[str, Any], field: str) -> Any: if not payload.get("epic_completed", True): return _MISSING return payload.get("epic") - if field == "issue_body": - # For ``new_issue`` work. There's no ``issue_body_completed`` - # flag in the result schema, so we use the same convention - # as ``description``: empty string maps to ``_MISSING``. - body = payload.get("issue_body") - if not body: - return _MISSING - return body raise ValueError(f"unknown --field: {field}") diff --git a/tools/local_ci_gate.sh b/tools/local_ci_gate.sh index 67016bf63..98bfe5a95 100755 --- a/tools/local_ci_gate.sh +++ b/tools/local_ci_gate.sh @@ -34,7 +34,17 @@ # # 0 — all selected gates passed # 1 — at least one gate failed -# 2 — argument error (bad flag, unknown gate) +# 2 — argument error (bad flag, unknown gate) OR no nox invocation +# resolvable (see pre-flight section below) +# +# Nox invocation resolution +# ------------------------- +# +# The script tries three resolution paths in order before falling back +# to a hard error. The auto-agents pipeline relies on path 3 (``uvx``) +# because the worker's throwaway ``/tmp/`` clone has no pre-installed +# Python tooling. Humans typically use path 1 or 2. See the +# ``Pre-flight`` block below for the exact order and diagnostics. # set -euo pipefail @@ -98,17 +108,65 @@ while [[ $# -gt 0 ]]; do esac done -# ─── Pre-flight ─────────────────────────────────────────────────────────── -if ! command -v nox >/dev/null 2>&1; then - printf 'ERROR: nox not on PATH. Install via "uv tool install nox" or pip.\n' >&2 - exit 2 -fi - # Resolve the workspace root (this script lives in tools/). SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" cd "$REPO_ROOT" +# ─── Pre-flight: resolve nox invocation ─────────────────────────────────── +# +# Three resolution paths, ordered from "human-developer happy path" to +# "throwaway-clone fallback": +# +# 1. ``nox`` already on PATH (most human developers, or someone who ran +# ``uv tool install nox`` once and persisted it). +# 2. ``${REPO_ROOT}/.venv/bin/nox`` exists (project venv created by the +# contributing workflow; ``pip install -e .[dev]`` brings nox in). +# 3. ``uvx`` on PATH (uv is system-installed; ``uvx nox`` ephemerally +# provisions nox per invocation, no persistent state required). This +# is the auto-agents pipeline's deterministic fallback when the +# worker has cloned the repo into ``/tmp/...`` with no Python +# tooling pre-installed. +# +# We capture the resolution as a bash array (NOX_CMD) so callers can +# substitute it transparently for the bare ``nox`` invocation later. +# Echo the chosen path to stderr so the operator (or the implementer +# worker's attempt comment) can see why a particular path was selected. +NOX_CMD=() +if command -v nox >/dev/null 2>&1; then + NOX_CMD=(nox) + printf '# local_ci_gate.sh — using system nox at %s\n' \ + "$(command -v nox)" >&2 +elif [[ -x "${REPO_ROOT}/.venv/bin/nox" ]]; then + NOX_CMD=("${REPO_ROOT}/.venv/bin/nox") + printf '# local_ci_gate.sh — using project venv nox at %s\n' \ + "${REPO_ROOT}/.venv/bin/nox" >&2 +elif command -v uvx >/dev/null 2>&1; then + # ``--quiet`` suppresses uvx's per-invocation provisioning banner so + # the gate banners stay readable. The first call downloads nox into + # uv's tool cache (~3 s on cold cache); subsequent calls reuse it. + NOX_CMD=(uvx --quiet nox) + printf '# local_ci_gate.sh — using uvx fallback (nox via %s)\n' \ + "$(command -v uvx)" >&2 +else + cat >&2 <<'EOF' +ERROR: cannot resolve a nox invocation. Tried, in order: + 1. nox on PATH — not found + 2. .venv/bin/nox in repo root — not found + 3. uvx on PATH — not found + +To fix, choose ONE: + - Install nox globally: uv tool install nox + - Install uv (gives uvx): curl -LsSf https://astral.sh/uv/install.sh | sh + - Create a project venv: python3 -m venv .venv && .venv/bin/pip install nox + +Auto-agents pipeline operators: the dispatcher's pre-clone is expected +to use whichever of these is available on the host. See the +``quality-gates`` skill for the worker-side recipe. +EOF + exit 2 +fi + # ─── Gate selection ─────────────────────────────────────────────────────── case "$mode" in all) @@ -137,7 +195,7 @@ failed=() for gate in "${selected[@]}"; do banner_start=$(date -u +%s) printf '\n## [%s] start\n' "$gate" >&2 - if nox -e "$gate"; then + if "${NOX_CMD[@]}" -e "$gate"; then elapsed=$(( $(date -u +%s) - banner_start )) printf '## [%s] PASS (%ds)\n' "$gate" "$elapsed" >&2 else