fix(auto-agents): quality-gate env bootstrap + filesystem-handoff hardening rounds 6-7
Two workstreams in one commit: 1. Quality-gate environment bootstrap + new quality-gates skill (round 8). The Tier 0 task-implementor was reporting "Failed — nox not available" on every PR because the worker's /tmp throwaway clone had no Python tooling. tools/local_ci_gate.sh now self-bootstraps nox via a three- step resolution chain (system PATH → project venv → uvx fallback) and exits 2 with an actionable diagnostic if none resolves. New .opencode/skills/quality-gates/SKILL.md gives the implementer worker a deterministic recipe + troubleshooting appendix. task-implementor.md adds the matching bash allow-rules (bash tools/local_ci_gate.sh *, uvx --quiet nox *) plus the skill on its allowlist; step 5 of the procedure now references the skill. 9 new unit tests in test_local_ci_gate.py pin the three-step resolution chain, the bad-shape error paths, and the "system-nox failure does not fall through to uvx" invariant. Verified end-to-end against a fresh /tmp clone with PATH restricted to system + uvx: both lint and typecheck gates ran cleanly via uvx fallback. 2. Filesystem-handoff hardening rounds 6 and 7 (P1/P2 follow-ups from the iterative critique loop). Round 6 propagated the round-5 single-source-of-truth pattern to the sentinel writer (_to_dict overlays COMPLETION_FLAG_NAMES) and the worker reader (--field metadata introspects payload keys), added the bidirectional drift guard test (tuple ↔ dataclass set-equality), and corrected round-5 CHANGELOG wording. Round 7 added defence-in-depth: an assert-based schema-base collision guard in _to_dict, schema-lock tests pinning the writer's full output key set, writer-side typo guard, and a reader self-adapts test for unknown future flags. Test results: 1,170 passing / 3 skipped (+10 vs the round-5 baseline, +9 in this round). Lint: 11 pre-existing errors across the changed files, unchanged baseline (corrects the round-6/7 entries' aspirational "7 pre-existing" claim — empirically the _to_dict refactor cleaned up zero lints). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -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 <name>``, ``--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 <gate>``
|
||||
# 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 <<EOF > /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).
|
||||
|
||||
`<field>` 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 <name>
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
|
||||
@@ -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 `<EMPTY_TOKEN>` 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 == "<EMPTY_TOKEN>":
|
||||
# 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` (`<EMPTY_TOKEN>` = `"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` (`<EMPTY_TOKEN>` = `"[]"`):
|
||||
|
||||
```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)
|
||||
|
||||
|
||||
@@ -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 <name>
|
||||
# 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 <name>` 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/<your-clone>/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/<your-clone>/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.
|
||||
```
|
||||
+608
-8
@@ -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 <gate>`` 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
|
||||
|
||||
@@ -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``.
|
||||
|
||||
@@ -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"
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 ``<path>/<name>.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/<user>/.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 <gate>`` 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 <bogus>`` 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."
|
||||
)
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
+115
-21
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
+125
-51
@@ -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}")
|
||||
|
||||
|
||||
|
||||
+66
-8
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user