Skip to content

fix: per-PR workspaces, realistic duration default, honest timeout message; restore the pipeline DAG in the Go port#54

Merged
AbirAbbas merged 4 commits into
mainfrom
fix/reliability-defaults-and-parallel-workspaces
Jul 14, 2026
Merged

fix: per-PR workspaces, realistic duration default, honest timeout message; restore the pipeline DAG in the Go port#54
AbirAbbas merged 4 commits into
mainfrom
fix/reliability-defaults-and-parallel-workspaces

Conversation

@AbirAbbas

@AbirAbbas AbirAbbas commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What

Four reliability fixes found by running pr-af-go in production against Agent-Field/agentfield's open PRs on Windows, applied to both ports (Python + Go, byte-identical behavior per the parity contract) — plus the restoration of the pipeline DAG in the Go port (item 5).

  1. Per-PR workspaces — parallel reviews no longer clobber each other. Both ports resolved every PR of a repo into ONE shared dir ($PR_AF_WORKDIR/<repoName>) whose pr-review branch was re-pointed per review, so concurrent reviews of different PRs silently explored the wrong checkout. The workspace is now keyed <repoName>-pr<N> when the PR number is known (plain <repoName> otherwise, preserving repo_path/diff_text flows). New tests in both ports pin two PRs of one repo → two dirs, each at its own PR head.

  2. PR_AF_MAX_DURATION_SECONDS default 300 → 3600. Real reviews measure 60–70 minutes; at the old 5-minute default every fresh install died mid-pipeline. Updated in code defaults, both manifests, .env.example, and both README env tables.

  3. Honest budget-exhaustion message. "Budget exhausted before " fired whether the cost cap or the wall-clock cap tripped — and in the Go port cost never accrues, so it was always wall-clock and the message was actively misleading. Duration trips now say Review time budget exceeded (max_duration_seconds=<N>) before <phase>; cost trips keep the old wording. Strings byte-identical across ports, pinned by new tests in both.

  4. Manifests declare AGENTFIELD_HARNESS_IDLE_SECONDS (default 360). Harness CLIs in JSON mode emit events only at completion boundaries, so one long reasoning completion is minutes of legitimate silence; the SDKs' old 120s idle watchdog killed healthy runs (observed live at 8–19 minutes into progressing pipelines).

  5. Go port: pipeline phases are tracked executions again — the review DAG is restored. The Python node reports every phase (intake_phase, anatomy_phase, the meta_* selectors, each parallel review_dimension, adversary_phase, evidence_verifier, the obligation/compound reasoners, coverage_gate) as a child execution of the parent review, and the control-plane UI renders the run as that orchestration DAG. The Go port invoked the same phase functions directly in-process, so the control plane only ever saw a single review node — the architecture's DAG had collapsed in the port. The orchestrator's phase seams now route through the SDK's Agent.CallLocal (under the registered reasoner names, shared as reasoners.Name* constants with register.go so they cannot drift): each call builds a child execution context and emits running/succeeded/failed workflow events that the control plane mirrors into the executions table — one DAG node per phase, parented under review, same graph shape as Python. Behavior is otherwise unchanged: a routed phase runs the same reasoners.* function with the same deps via the registered handler; the new afx.ToMap keeps nested values typed so OrderedPatches' insertion-ordered diff_patches survives the round trip byte-for-byte (a naive JSON round trip re-sorts it — caught by the new round-trip tests); events are fire-and-forget, so an unreachable control plane never fails a review; and orch.Deps.Local stays nil in unit tests/stub harnesses, preserving the direct-call seams there.

Plus: Go SDK bumped to v0.0.0-20260714191100-2cc5fe2adcf4, which carries the Windows fix where opencode prompts went over argv and blew cmd.exe's ~8k cap — on Windows that silently produced zero-dimension "Safe to merge" reviews (Agent-Field/agentfield#774 has the full story). Re-bump to a tagged SDK once #774 merges if preferred.

Validation

  • Go: go build / go vet / gofmt clean; full go test ./... green, including the new DAG-routing suites (internal/orch/calllocal_test.go, internal/node/calllocal_wiring_test.go, afx.ToMap tests) which pin: every phase seam calls CallLocal under its registered name; the input map binds back into the identical typed input (including deliberate zero values on fields with non-zero bind-side defaults, and diff_patches insertion order); errors propagate unchanged; nil Deps.Local keeps the direct seams.
  • Python: full suite 73 passed including the 4 new tests; ruff check clean.

Field data behind the defaults

From the production run that motivated this: reviews of 335–2100-line PRs took 60–70 min end-to-end on kimi-k2.5/deepseek-v4-pro via opencode; the 300s cap killed 100% of attempts; the 120s idle watchdog killed 4 runs 8–19 minutes in; two concurrent same-repo reviews raced the shared workspace until isolated manually.

🤖 Generated with Claude Code

AbirAbbas and others added 4 commits July 14, 2026 15:43
Four reliability fixes from live Windows testing, applied to BOTH ports
(Python src/pr_af and the byte-compatible Go port) together:

1. Per-PR workspaces (parallel safety). _resolve_repo / ResolveRepo now
   key the checkout by repo AND PR number (<repoName>-pr<N>) when a PR
   number is known. The shared per-repo dir re-pointed its pr-review
   branch on every review, so two concurrent reviews of different PRs of
   the same repo silently reviewed the wrong checkout. Plain <repoName>
   is kept when no PR number is known (repo_path/diff_text flows). New
   tests in both ports assert two PRs of the same repo resolve to
   different directories, each at its own PR head.

2. Realistic duration default. PR_AF_MAX_DURATION_SECONDS now defaults
   to 3600 (was 300); real reviews measure 60-70 minutes, so every fresh
   install died mid-pipeline. Changed in the resolve-caps cascade,
   input-schema and BudgetConfig defaults of both ports, both manifests,
   .env.example, both README env tables, and the docs.

3. Honest budget-exhaustion message. "Budget exhausted before <phase>"
   was reported whether the cost cap or the wall-clock cap tripped (in
   the Go port cost never accrues, so it was ALWAYS the wall clock).
   When the duration cap trips the message is now "Review time budget
   exceeded (max_duration_seconds=<N>) before <phase>"; the cost cap
   keeps the historical wording. New §B.4 string is pinned byte-
   identically by tests in both ports.

4. Declare the harness idle watchdog. AGENTFIELD_HARNESS_IDLE_SECONDS
   (default "360") added to user_environment.optional in both manifests
   and both README env tables: harness CLIs in JSON mode emit events
   only at completion boundaries, so long single completions look
   silent — the old 120s SDK default killed healthy runs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Picks up fix/harness-windows-reliability — the Windows stdin fix for
opencode prompts (v0.0.0-20260714191100-2cc5fe2adcf4).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ew DAG

The Python node reports every pipeline phase (intake_phase, anatomy_phase,
meta_* selectors, each review_dimension, adversary_phase, ...) as a tracked
child execution of the parent review — the control-plane UI renders the run
as that orchestration DAG. The Go port called the phase functions directly
in-process, so the control plane only ever saw a single review execution:
the architecture's DAG collapsed into one opaque node.

Route the orchestrator's reasoner seams through the SDK's Agent.CallLocal
when orch.Deps.Local is wired (production always wires the live agent):
each phase invocation now builds a child execution context and emits
running/succeeded/failed workflow events under its registered reasoner name,
which the control plane mirrors into the executions table — one DAG node per
phase, parented under review, same shape as Python.

- reasoners: add Name* constants as the single source of truth for the
  registered names; register.go and the CallLocal seams share them so DAG
  phase names cannot drift from the registered surface.
- afx: add ToMap (Bind's inverse). It keeps nested field values typed
  instead of JSON-round-tripping the whole struct — a plain round trip
  flattens OrderedPatches into a Go map and silently re-sorts diff_patches,
  breaking the insertion order the meta/obligation prompts render.
- orch: LocalCaller interface + Deps.Local; callLocalSeams/viaLocal adapt
  each typed seam to CallLocal under its registered name. nil Local keeps
  the direct-call seams (unit tests, stub harnesses).
- node: BuildAgent wires localCaller to the live agent; reviewHandler
  forwards it as Deps.Local.

Behavior is otherwise unchanged: a CallLocal-routed phase runs the same
reasoners.* function with the same deps via the registered handler, events
are fire-and-forget (an unreachable control plane never fails a review),
and the ToMap->Bind round trip is covered by tests including deliberate
zero values on fields with non-zero bind-side defaults.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas AbirAbbas changed the title fix: per-PR workspaces, realistic duration default, honest timeout message (both ports) fix: per-PR workspaces, realistic duration default, honest timeout message; restore the pipeline DAG in the Go port Jul 14, 2026
@AbirAbbas
AbirAbbas merged commit 6b82efc into main Jul 14, 2026
3 checks passed
@AbirAbbas
AbirAbbas deleted the fix/reliability-defaults-and-parallel-workspaces branch July 14, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant