0b657cd0d9
Post-commit review of d386ff4e surfaced two real bugs and several
rough edges. None changed the architecture — all changes harden the
existing dispatcher↔worker filesystem-handshake contract.
P0 bug fixes
- Three-case read contract for implementer_pr_context.py. The old
``or None`` projection conflated "field missing" with "field
present but empty," forcing the worker to re-curl Forgejo every
time the dispatcher had already confirmed a section was empty.
New contract: empty stdout = "didn't try; fall through to legacy
GET"; ``null\n`` = "tried and authoritatively empty; SKIP GET";
any other content = use it.
- ``comments`` field dispatches on ``work_type`` instead of using
the ``pr_comments or issue_comments`` chain. The old code would
silently leak ``issue_comments`` from a stale issue context into
a ``pr_fix`` worker's ``--field comments`` read.
- Every section's projection now honours its ``*_completed`` flag.
A failed upstream fetch (transient API error) maps to empty
stdout instead of authoritative empty data.
P1 hardening
- Dropped ``_resolve_branch_for_sha``. The pre-clone path was
shelling out to ``git for-each-ref --points-at <sha>`` for data
the dispatcher already had from ``pr_details.head.ref``. Now
``prepare_pr_worktree`` takes ``head_ref`` as a kwarg.
- Both writers (PR-context and workspace sentinels) clean up
their ``.tmp`` orphan files on partial-write / serialisation
failure.
- Removed the dead ``cleanup`` subcommand from
tools/implementer_workspace.py — worktree cleanup is the
dispatcher's job (WorktreeHandle.cleanup); the worker has no
legitimate reason to rm -rf a worktree mid-session.
- Tightened bash allow-rules in task-implementor.md from
``<script> *`` to ``<script> <subcommand> *`` so future
subcommands require explicit operator review.
- Retired the prompt-vs-sentinel "use either" softener in
task-implementor.md and the implementer-pr-context SKILL.md.
The scripts are now documented as the SINGLE SOURCE OF TRUTH.
Test additions
- 5 new dispatcher↔sentinel integration tests in
test_dispatch_implementer.py: writer call site, new_issue
work_type mapping, cleanup integration with and without a
context dict, partial-fetch completion-flag propagation.
- 5 new contract tests in test_implementer_pr_context_cli.py:
the three-case epic contract, work_type dispatch in both
directions, failed-fetch fall-through.
- 2 new sentinel writer tests in test_pr_context_sentinel.py:
``.tmp`` orphan cleanup paths, real ImplementerPrefetchResult
round-trip (defends against silent-attribute-miss when fields
are added to the dataclass).
- ``test_workspace_handoff.py`` integration test now asserts NO
``git for-each-ref`` invocation (regression guard for the
dropped helper).
Full auto_agents suite: 1,128 passed, 3 skipped (was 1,123 before).
Co-authored-by: Cursor <cursoragent@cursor.com>
392 lines
15 KiB
Python
Executable File
392 lines
15 KiB
Python
Executable File
#!/usr/bin/env python3
|
|
"""Worker-side reader for the dispatcher's PR-context handoff sentinel.
|
|
|
|
What this is
|
|
------------
|
|
|
|
The dispatcher pre-fetches everything the implementer worker needs to
|
|
make a code change — PR description, full diff, CI status + per-check
|
|
detail, PR comments, active REQUEST_CHANGES reviews, linked issues,
|
|
parent Epic body — and writes the lot to a sentinel at
|
|
``/tmp/cleveragents-implementer-handoff/pr-{N}.json``. The dispatcher
|
|
ALSO embeds those sections in the wrapper session's user prompt, but
|
|
each ``task`` tool call inside OpenCode re-summarises the prompt for
|
|
the child agent — by the time the chain reaches ``task-implementor``
|
|
at depth 3, only the diff section reliably survives.
|
|
|
|
This script is what the implementer worker calls (via the
|
|
``implementer-pr-context`` skill) to re-read any prefetched section
|
|
on demand, regardless of how the prompt got mangled upstream. It is
|
|
the read half of :mod:`tools._pr_context_sentinel`.
|
|
|
|
Contract
|
|
--------
|
|
|
|
Subcommand ``read``:
|
|
|
|
``python3 tools/implementer_pr_context.py read --pr <N> --field <name>``
|
|
|
|
Reads the sentinel at ``/tmp/cleveragents-implementer-handoff/pr-{N}.json``,
|
|
pulls the requested field, prints it on stdout, and exits 0. The
|
|
``--field`` value selects which slice of the prefetched data:
|
|
|
|
- ``description`` — the PR body text (plain string)
|
|
- ``title`` — the PR title (plain string)
|
|
- ``metadata`` — JSON with work_type / work_group / head_sha /
|
|
head_ref / base_ref / pr_number / data_complete / error_kinds
|
|
- ``ci`` — JSON with the latest CI ``state`` plus ``checks[]``
|
|
(per-status detail, including ``target_url`` and any prefetched
|
|
failure-log excerpts)
|
|
- ``comments`` — JSON list of PR / issue comments (chronological)
|
|
- ``reviews`` — JSON list of active REQUEST_CHANGES reviews + their
|
|
inline comments (for ``request_changes_pr`` work only)
|
|
- ``issues`` — JSON list of linked issues (the ``Closes #N``
|
|
targets, with their bodies inlined)
|
|
- ``epic`` — JSON dict with the parent Epic body, or empty if no
|
|
Epic reference was found
|
|
- ``diff`` — the unified diff as plain text (between
|
|
``BEGIN_PR_DIFF`` / ``END_PR_DIFF`` markers in the original
|
|
prompt)
|
|
- ``all`` — the entire sentinel payload as JSON (useful for
|
|
debugging; ``task-implementor`` should pull narrowly)
|
|
|
|
The script emits one of three signals on stdout, all with exit
|
|
code 0:
|
|
|
|
- **Empty stdout** — the sentinel is missing, malformed, or the
|
|
dispatcher's fetch for this field did not complete (per the
|
|
matching ``*_completed`` flag). The worker should fall through
|
|
to its legacy curl-based GET against the live Forgejo API.
|
|
- ``null\\n`` — the dispatcher fetched successfully and
|
|
authoritatively confirmed absence (e.g. the PR has no Epic
|
|
reference). The worker should SKIP its legacy GET; there is
|
|
nothing to fetch.
|
|
- Any other content — the field's value (plain text for
|
|
description/title/diff/issue_body, pretty-printed JSON for
|
|
everything else). The worker should use it verbatim.
|
|
|
|
This three-case contract is how the worker distinguishes "the
|
|
dispatcher didn't try" (fall through) from "the dispatcher tried
|
|
and confirmed there is nothing" (don't re-curl). Conflating the
|
|
two — as the original ``or None`` projection did — caused the
|
|
worker to redundantly hit Forgejo for sections the dispatcher
|
|
had already verified empty.
|
|
|
|
Exit codes
|
|
----------
|
|
|
|
- 0: success (or empty stdout for "no data available" — see above).
|
|
- 2: usage error (bad / missing CLI arg, unknown ``--field``).
|
|
- 3: schema-version mismatch (a config issue the operator must
|
|
notice; the worker should treat this as "no data available" but
|
|
the non-zero code makes the situation visible in logs).
|
|
|
|
Allow-rule
|
|
----------
|
|
|
|
This script's path is whitelisted under
|
|
``python3 tools/implementer_pr_context.py *`` in the agent's
|
|
permission allowlist (see :file:`.opencode/agents/task-implementor.md`).
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import argparse
|
|
import json
|
|
import os
|
|
import sys
|
|
from pathlib import Path
|
|
from typing import Any
|
|
|
|
|
|
# Must match :data:`tools._pr_context_sentinel.SCHEMA_VERSION`.
|
|
EXPECTED_SCHEMA_VERSION = 1
|
|
|
|
_DEFAULT_HANDOFF_DIR = Path("/tmp/cleveragents-implementer-handoff")
|
|
|
|
|
|
def _handoff_dir() -> Path:
|
|
"""Resolve the on-disk directory. Mirrors
|
|
:func:`tools._pr_context_sentinel.handoff_dir`."""
|
|
return Path(
|
|
os.environ.get("IMPLEMENTER_DISPATCHER_PR_CONTEXT_DIR")
|
|
or str(_DEFAULT_HANDOFF_DIR)
|
|
)
|
|
|
|
|
|
def _handoff_path(pr_number: int) -> Path:
|
|
return _handoff_dir() / f"pr-{int(pr_number)}.json"
|
|
|
|
|
|
def _load(pr_number: int) -> tuple[dict[str, Any] | None, str | None]:
|
|
"""Read + parse + validate the sentinel.
|
|
|
|
Returns ``(payload, error)``:
|
|
|
|
- ``(payload, None)`` — sentinel exists, parses, schema matches.
|
|
- ``(None, None)`` — sentinel does not exist (normal: dispatcher
|
|
didn't prefetch, or already cleaned up).
|
|
- ``(None, error)`` — sentinel exists but is unusable (malformed,
|
|
schema mismatch). The caller emits the error to stderr.
|
|
"""
|
|
target = _handoff_path(pr_number)
|
|
if not target.exists():
|
|
return None, None
|
|
try:
|
|
raw = target.read_text(encoding="utf-8")
|
|
payload = json.loads(raw)
|
|
except (OSError, ValueError) as e:
|
|
return None, f"handoff read/parse failed at {target}: {e}"
|
|
if not isinstance(payload, dict):
|
|
return None, f"handoff at {target} is not a JSON object"
|
|
schema = payload.get("schema_version")
|
|
if schema != EXPECTED_SCHEMA_VERSION:
|
|
return None, (
|
|
f"handoff at {target} has schema_version={schema!r}, "
|
|
f"expected {EXPECTED_SCHEMA_VERSION}"
|
|
)
|
|
return payload, None
|
|
|
|
|
|
# Sentinel for "field is not present in this handoff" / "fetch failed
|
|
# upstream". Distinct from ``None`` so the worker contract can
|
|
# distinguish three cases:
|
|
#
|
|
# - ``_MISSING`` → emit NOTHING (empty stdout); worker falls through
|
|
# to its legacy GET. Used when the dispatcher didn't attempt this
|
|
# section (legacy prompt path) OR when the dispatcher attempted
|
|
# and failed (transient error reflected in the matching
|
|
# ``*_completed=False`` flag).
|
|
# - ``None`` → emit the JSON literal ``null`` (stdout = ``null\n``).
|
|
# Used when the dispatcher fetched successfully and confirmed
|
|
# absence (e.g. the PR has no Epic reference). The worker sees
|
|
# non-empty stdout and skips its legacy GET, treating "null" as
|
|
# the authoritative "no data" answer.
|
|
# - any other value → emit it; worker uses it verbatim.
|
|
#
|
|
# Without this distinction the worker re-curls the live Forgejo API
|
|
# every time the dispatcher confirmed an absence, defeating the
|
|
# whole point of the handoff.
|
|
_MISSING: Any = object()
|
|
|
|
|
|
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.
|
|
|
|
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.
|
|
"""
|
|
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
|
|
if field == "title":
|
|
title = payload.get("title")
|
|
if not title:
|
|
return _MISSING
|
|
return title
|
|
if field == "diff":
|
|
diff = payload.get("diff")
|
|
if not diff:
|
|
return _MISSING
|
|
return diff
|
|
if field == "metadata":
|
|
return {
|
|
"schema_version": payload.get("schema_version"),
|
|
"pr_number": payload.get("pr_number"),
|
|
"work_type": payload.get("work_type"),
|
|
"work_group": payload.get("work_group"),
|
|
"head_sha": payload.get("head_sha"),
|
|
"head_ref": payload.get("head_ref"),
|
|
"base_ref": payload.get("base_ref"),
|
|
"listing_title": payload.get("listing_title"),
|
|
"data_complete": payload.get("data_complete"),
|
|
"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.
|
|
return {
|
|
"status": payload.get("ci_status"),
|
|
"checks": payload.get("ci_detail") or [],
|
|
"status_completed": payload.get("ci_status_completed", True),
|
|
"detail_completed": payload.get("ci_detail_completed", True),
|
|
}
|
|
if field == "comments":
|
|
# Dispatch on ``work_type``: pr_fix / request_changes_pr
|
|
# cycles populate ``pr_comments``; new_issue cycles populate
|
|
# ``issue_comments``. Mixing them was a latent bug — if a
|
|
# future change ever populated both (e.g. an issue_impl
|
|
# 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 []
|
|
if field == "reviews":
|
|
if not payload.get("request_changes_reviews_completed", True):
|
|
return _MISSING
|
|
return payload.get("request_changes_reviews") or []
|
|
if field == "issues":
|
|
if not payload.get("linked_issues_completed", True):
|
|
return _MISSING
|
|
return payload.get("linked_issues") or []
|
|
if field == "epic":
|
|
# ``epic_completed=False`` means the fetch failed; fall
|
|
# through. ``epic_completed=True`` with ``epic=None`` means
|
|
# the dispatcher confirmed the PR has no Epic reference —
|
|
# emit ``null`` so the worker skips its legacy GET.
|
|
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}")
|
|
|
|
|
|
_ALLOWED_FIELDS = (
|
|
"all",
|
|
"description",
|
|
"title",
|
|
"diff",
|
|
"metadata",
|
|
"ci",
|
|
"comments",
|
|
"reviews",
|
|
"issues",
|
|
"epic",
|
|
"issue_body",
|
|
)
|
|
|
|
|
|
def _emit(value: Any, field: str) -> None:
|
|
"""Render ``value`` to stdout in the format the worker expects.
|
|
|
|
Three cases (see :data:`_MISSING` docstring for full rationale):
|
|
|
|
- ``value is _MISSING`` → emit nothing. The worker reads empty
|
|
stdout and falls through to its legacy GET.
|
|
- ``value is None`` → emit ``null\\n``. The worker reads
|
|
non-empty stdout and skips its legacy GET (the dispatcher
|
|
authoritatively confirmed absence).
|
|
- any other value → emit verbatim. Strings print as plain text
|
|
with a trailing newline; everything else prints as
|
|
pretty-printed JSON.
|
|
"""
|
|
if value is _MISSING:
|
|
return
|
|
if value is None:
|
|
# Authoritative "no data" answer (e.g. dispatcher confirmed
|
|
# no Epic). The worker contract reads this as "skip the
|
|
# legacy GET; there's nothing to fetch." JSON ``null`` is
|
|
# parseable in any language the worker might use.
|
|
sys.stdout.write("null\n")
|
|
return
|
|
if isinstance(value, str):
|
|
# Plain-text contract for these fields so the worker can
|
|
# pipe them straight into a file or display them inline
|
|
# without an extra json parse.
|
|
sys.stdout.write(value)
|
|
# Ensure a trailing newline so terminal usage looks right.
|
|
if not value.endswith("\n"):
|
|
sys.stdout.write("\n")
|
|
return
|
|
# Structured fields: pretty-print JSON so the worker (or an
|
|
# operator inspecting from the shell) gets a readable blob.
|
|
json.dump(value, sys.stdout, indent=2, default=str)
|
|
sys.stdout.write("\n")
|
|
|
|
|
|
def _cmd_read(args: argparse.Namespace) -> int:
|
|
"""Implement ``read``. Always exits 0 on the "no data" path —
|
|
that's a normal condition (e.g. epic-field on a PR with no
|
|
Epic reference). Exit 3 only on schema mismatch, which is a
|
|
config issue the operator must notice."""
|
|
payload, error = _load(int(args.pr))
|
|
if error:
|
|
print(f"warning: {error}", file=sys.stderr)
|
|
if "schema_version" in error:
|
|
return 3
|
|
return 0
|
|
if payload is None:
|
|
# No sentinel — the dispatcher didn't prefetch (or cleaned
|
|
# up already). Empty stdout is the "no data" signal.
|
|
return 0
|
|
try:
|
|
value = _project_field(payload, args.field)
|
|
except ValueError as e:
|
|
print(f"error: {e}", file=sys.stderr)
|
|
return 2
|
|
_emit(value, args.field)
|
|
return 0
|
|
|
|
|
|
def main(argv: list[str] | None = None) -> int:
|
|
parser = argparse.ArgumentParser(
|
|
prog="implementer_pr_context",
|
|
description=(
|
|
"Worker-side reader for the dispatcher's PR-context "
|
|
"handoff sentinel. See module docstring for the full "
|
|
"contract."
|
|
),
|
|
)
|
|
sub = parser.add_subparsers(dest="cmd", required=True)
|
|
|
|
p_read = sub.add_parser(
|
|
"read",
|
|
help=(
|
|
"Read one field from the PR's context handoff. Prints "
|
|
"the field on stdout (plain text for description/title/"
|
|
"diff/issue_body, JSON for everything else). Empty "
|
|
"stdout means 'no data available — fall back to curl'."
|
|
),
|
|
)
|
|
p_read.add_argument(
|
|
"--pr", required=True, type=int, help="PR number"
|
|
)
|
|
p_read.add_argument(
|
|
"--field",
|
|
required=True,
|
|
choices=_ALLOWED_FIELDS,
|
|
help="Which slice of the prefetched data to print",
|
|
)
|
|
|
|
args = parser.parse_args(argv)
|
|
if args.cmd == "read":
|
|
return _cmd_read(args)
|
|
parser.error(f"unknown subcommand {args.cmd!r}")
|
|
return 2 # pragma: no cover — argparse exits before reaching here
|
|
|
|
|
|
if __name__ == "__main__":
|
|
sys.exit(main())
|