Repository navigation
feat(workflow): add durable pull request delivery - #1306
Conversation
- Add deliver: pr for writable leaves, composite branches, fan-out items and loop passes. Persist post-agent delivery separately, serialize provider I/O outside the run lock, retain cancellation receipts and retry failed delivery without rerunning the agent or losing the pushed branch. - Record copy origins and resolve stacked targets through after dependencies, skipped/read-only steps and inherited workflow needs. Reuse open requests through existing GitHub, GitLab, TGit, GitCode and CNB authentication; retain the generic Git fallback and never invoke a provider merge API. - Inspect the exact delivered commit against the run base before PR creation, reject protected CI/agent/MCP/rules/hook paths, and preserve execution bounds across delivery. Reconcile limits reached by a report before provider I/O. - Validate and sanitize driver review URLs without exposing rejected values, report unknown changes fields by name, and publish ordered, deduplicated prs from validated changes only. Keep usage-report opt-out and done envelopes. - Document delivery and the unattended CI path in both languages, update help and dashboard state, and verify 1,311 related unit tests, 40 CLI E2E cases, focused provider/recovery/security tests, typecheck, lint and combined requirements/session/delivery integration against local fixture remotes.
The PR description supplies sufficient testing, including a build-first real-CLI record at |
- Carry both a workflow step timeout and its nested workflow timeout into the delivery deadline, keeping the earliest bound rather than preferring one. - Keep the root and lease deadlines in the same minimum so delivery cannot relax the constraints established by the requirements implementation. - Verify all 29 delivery regressions, typecheck, lint and the build-first real CLI PR creation/reuse, generic-host recovery and required-approval cases.
The description documents sufficient testing, including build-first real-CLI verification at No additional confident findings. Only the scoped diff was read; no PR code was run, built, or installed. |
- Reload the durable run and verify the same delivering pass/attempt after an open-request lookup returns no match, before starting any PR creation request. Cancellation during lookup now prevents creation; already in-flight creation still retains its safe receipt without reviving cancelled work. - Reject deliver: pr on commands with an implicitly read-only workspace, while retaining prompts that inherit a writable enclosing workflow. Clarify the command requirement in both language guides. - Add a regression that cancels during lookup and previously observed an unwanted create call, plus parser coverage for implicit read-only commands and valid inherited prompt workspaces. - Verify 118 delivery/parser/provider/guide tests, typecheck, lint and a build-first real CLI delivery, reuse, recovery and approval round trip.
Both earlier findings are resolved for their reported scenarios: cancellation is rechecked after lookup, and commands without an explicit writable workspace are rejected. The description documents sufficient build-first real-CLI testing. The older commit reference and deferred live-provider coverage are not blocking. Only the scoped diff was read; no PR code was run, built, or installed. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@nateEc Thanks. This branch conflicts with main in src/workflow/types.ts and src/__tests__/workflow-records.test.ts. Main caused it: #1234 (2401d047) added workflow_namespace_conflict and prompt_namespace_conflict to the WorkflowDiagnostic['code'] union and to the diagnostics list in the records test, on the same lines this PR extends. Keeping both sets of codes should resolve it. We'll review the rebased head. #1303, #1304 and #1306 also share most of their workflow files (types.ts, parse.ts, local-driver.ts, src/index.ts, the workflow guides), so whichever lands first will need the other two rebased again.
@nateEc an update to the request for changes: the conflict with |
The snapshot union was a hand-kept copy of the run union, so a status
added to one and missed in the other compiled and then failed every
publish with snapshot_unsupported. Both unions now come from one variant
list; only the leased variant differs.
The in-progress check in settleScope and the cancellable check in cancel
were hand-written status chains whose miss fails at run time
(run_unreadable, not_cancellable). They are now exhaustive predicates, so
a new status does not compile until each one places it.
Red: "a pending step is in progress: false, and can be cancelled: true"
failed with "(0 , inProgress) is not a function". Dropping a status from
the snapshot union makes the parity test fail in tsc (TS2344) and at run
time ("expected [ 'cancelled', 'failed', ... ] to deeply equal [ ...,
'delivering', ... ]").
A top-level prompt with deliver: pr and no workspace runs read-only, so
it passed validation and then failed at delivery after the agent had
spent its tokens. One file cannot decide this: the same prompt inside a
sub-workflow continues the copy of the workspace: new step that runs it.
workflow start now resolves each delivering step's effective workspace
on the whole tree and refuses with bad_delivery, naming the step. A
workflow step that delivers must own its branch with workspace: new.
A pull request targets a branch, and a run started from a detached HEAD
has none, so a tree that delivers is refused at start with
no_base_branch, telling to check out a named branch.
Per-file diagnostics keep what no enclosing step can change (a value
other than pr, explicit read-only, a bare command) and now name the step
and the fix.
Red: "refuses a top-level prompt that delivers but runs read-only" and
"refuses a run that delivers from a detached HEAD" failed with "expected
{ data: { ... } } to deeply equal { error: { ... } }": the run started.
The parse case failed on the old message "deliver must be pr on a
writable branch".
…next
A step with deliver: pr now settles its agent's report into delivering,
and the next call of any driver delivers it before it answers. The run
lock is held only to pick a delivery and to apply its outcome, never
across a provider, and a pid-owned delivery lock per run keeps two
processes of one machine from delivering the same step twice. A process
that finds the lock held answers wait with the steps still delivering,
and the local driver polls rather than giving up as busy.
A delivered pull request is recorded once on the branch it was opened
from, under copies[branch].pr, so a retry or a second pass cannot open
another, and the end record lists those as prs in step and pass order.
A record merged for a run that ends again without prs drops the old
list. The report hands back cleanup: delivery reads the pushed branch,
so the worktree is no longer kept for it.
Until TeamAI opens pull requests itself, a step with no reported URL
fails with delivery_failed naming the branch that keeps the work and the
retry command. The deadline the contributor computed from timeout fields
main does not have is removed with the delivery module that used it.
Red: "delivers a driver-reported URL without opening a PR, and leases the dependent in the same next" failed with "expected { data: { accepted: true, …(2) } } to deeply equal { data: { accepted: true, …(3) } }": the report delivered inline, failed the step at revision 3, and held back cleanup. "keeps the local driver polling while another process delivers" failed with "expected { error: { code: 'busy', …(1) } }", and the two prs record tests failed against the previous records module.
A driver that reports changes.pr_url learns when it was not kept: the
report is still accepted, and the envelope lists the field under
dropped with a reason and never the value, since it may hold a token.
A URL from a step without deliver: pr is dropped the same way rather
than discarded in silence, as nothing would ever read it.
The sanitizer table now asserts the reason for each dropped row, and
the fast-check properties generate credentials, queries and fragments,
which the previous generator never produced, so the properties that
none of them survive could not fail. The credentials case runs on a
delivering step and checks the end record as well as the envelope and
the run file.
The conformance report operation takes the extra changes fields
directly, so its harnesses parse the saved lease instead of asserting
its workspace exists.
Red: "drops a URL from a step that does not deliver, saying why, and still accepts the report" failed with "expected { data: { accepted: true, …(3) } } to deeply equal { data: { accepted: true, …(4) } }" (no dropped entry). The strengthened properties failed with counterexample "https://a.aa?a" against a sanitizer that kept the query.
CI and hook configuration run with someone's credentials, and agent
configuration steers the next agent, so a pull request TeamAI delivers
may not change either. Delivery now compares the delivered head with the
run base in the run's checkout, fetching the run's branches when the
head is not there yet, and fails with protected_path naming the files
before any provider request. A URL a driver reported is checked the same
way, so no step succeeds on such a branch. Changes a branch inherited
from a step it needs count as well, since the diff is against the base.
The protected list has one owner in worktree.ts: AGENT_PATHS, which
steps already read from the run base, plus DELIVERY_ONLY_PATHS for CI
and hooks, plus AGENTS.md and CLAUDE.md at any depth. The head comes
from a driver's report, so it is read only as a full commit id; a ref
name such as main would otherwise diff another tree and pass.
The reported-URL conformance scenario now stops at what a driver sees,
the report and the URL kept on the delivering step, because its runs
have no repository to read the branch from; real-git tests cover the
delivery itself.
Red: "refuses a branch that changes protected paths, naming them, before any provider request" failed for both cases, with "expected [ { id: 'fix', …(9) }, …(1) ] to match object" (delivery_failed rather than protected_path) and, for a reported URL, "expected { data: { lease: … } }" (the step succeeded and review was leased); "protect every path …" failed with "DELIVERY_ONLY_PATHS is not iterable". The full commit id test failed with "expected { data: { lease: … } }" with the id check removed.
A pull request recorded once per branch could not record a second
request opened after a person closed the first, and it read as a cache
of what was still open. The run now keeps an append-only list of
receipts, deliveries, one per URL in the order each first appeared, with
the branch, base branch, step and pass that delivered it. Whether a
delivery reuses an open request stays the host's lookup to answer. The
end record lists the receipt URLs in that order, and the snapshot
carries them to a member who claims the run.
Completion is applied to the job as it was picked, and tells the drain
when a receipt reached a run that had already ended, for example one
cancelled while its delivery was being checked. The drain then queues
the end record again under the run lock, so the published record lists
the pull request, and publishes it once the lock is released.
A delivery job now carries the checkout, base commit and base branch
it needs, and a delivering step whose run lacks them fails under the
lock instead of reaching the delivery code.
Red: "delivers a driver-reported URL without opening a PR, and leases the dependent in the same next", "records a pull request once, however many deliveries report it" and "records a pull request delivered after the run ended, and queues the end record again with it" failed with "expected undefined to deeply equal [ { …(5) } ]" (no deliveries on the run); the records test failed with "expected undefined to deeply equal [ …(3) ]". The handover test failed with "expected false to deeply equal" against a snapshot schema without deliveries.
A step with deliver: pr whose driver reported no URL failed delivery even
when the origin was on GitHub. Delivery now resolves the provider from the
checkout's configured remote.origin.url. It looks up the open request of the
delivered branch into the run base branch, and opens one only when there is
none. A create that fails is looked up again, since the host may have
opened the request before it failed or timed out.
Right before the create, the run is asked under its lock whether the step
still delivers this job. If it does, the step is marked `creating` in the
same write, so a request opened by a process that then stopped can be found
rather than opened twice. A cancelled step keeps its delivery for the same
reason. The create runs outside the member checkout and names the head
branch, so nothing is pushed from it. The lookup never prompts or logs in.
No provider error text is stored, because it may hold a credential.
Providers expose a read-only `pullRequests` lookup with a single `findOpen`
member. GitHub implements it. Generic git and the hosts without a lookup
yet are null, and delivery there fails with delivery_failed, naming the
branch kept on origin and the retry command. The per-provider workflow-pr
modules are removed. A structural test fails if any provider file or the
delivery code gains a merge, accept or auto-merge call, and pins the
lookup's key set.
Red: "opens one pull request from the pushed branch into the run base
branch", both "records the pull request ... rather than opening another"
cases, "keeps the work on origin when GitHub cannot be reached" and "drops a
URL with credentials" failed with `no lease: {"data":{"done":{...,"outcome":
"failed"}}}`, since delivery failed for want of a provider; "asks the run
before it opens a pull request" failed with `expected { ok: false, …(1) } to
deeply equal { ok: false, …(1) }`; "cancels a delivering step" failed with
`expected [ { id: 'fix', …(4) }, …(1) ] to match object [ { id: 'fix', …(3) },
…(1) ]`; the GitHub lookup tests failed with "GitHub has no pull request
lookup", the origin tests with "(0 , providerForRemote) is not a function",
and the generic git test with "expected undefined to be null". The merge
guard failed listing gh-cli.ts against two patterns with a local
`ghExec(['pr', 'merge', n, '--squash'])`, and its key-set check failed tsc
with TS2344 "Expected: literal string: findOpen, Actual: literal string:
merge" with a local `merge?` member.
…hen it is the branch's
A driver could report any sanitized URL on the origin's host for a step
with deliver: pr. Delivery then recorded it as the step's pull request,
even when it named another branch's request or no open request at all.
Before the run lock is taken, the report now asks the origin's provider
for the open request of the reported branch into the run base branch. A
URL on that host is kept only when it equals that request. Otherwise it is
dropped with a reason that names the branch, and delivery finds or opens
the branch's request itself, so a dropped alternate route of the same
request costs nothing. A URL on another host, an origin only git reaches,
or a lookup that cannot answer keeps the URL as sanitized. No per-provider
URL parsing is involved.
The protected-path test with a reported URL now seeds its open request, so
the URL is actually kept. It asserts that nothing is opened or accepted,
rather than that no provider is asked, since the report-time lookup only
reads. Tests that report a github.com URL against the fixture origin now
use the fake gh, so none of them reaches GitHub.
Red: both "drops a URL on the origin host that ..." cases failed with
`expected { data: { accepted: true, …(3) } } to match object { data: {
accepted: true, …(2) } }`, since the URL was kept and nothing was dropped.
A copy records the run branch it started from when it is first leased, so a
pull request from it can later target the nearest delivered branch it grew
from. The field was published by the snapshot rules but the snapshot format
had no place for it, so any run where one step's copy starts from another
step's branch could not be published or claimed.
The field is now `from`, the name the delivery target walk reads, with a
place in the snapshot. A claim refuses a snapshot whose copy names a `from`
outside the run, as it already refuses a copy branch outside the run, since a
pull request may target that branch.
Red: "hands over a copy that started from the branch of a step it needs, with
the branch it started from" failed with snapshot_unsupported on
copies.teamai/<run>/b/p1.origin; "refuses to claim a snapshot whose copy
started from a branch outside the run" failed with the claim accepted
(`expected { ok: true, … } to match object { ok: false, … }`).
… from A delivered branch now targets the nearest branch it started from that delivered a pull request, else the run base branch, so a stacked step's request holds only its own work. The walk follows the `from` each copy keeps, which already orders `after` items before other needs, passes over a skipped need to that need's own needs, and continues a sub-workflow's first step through the workflow step's needs. A reported URL on the origin host is checked against the same target. Tests cover the behaviour already in place too: a composite workflow step opens one request for its shared branch and none for its steps, a looped `new` step opens one per `/p<pass>` branch and leaves the earlier ones open, a loop on one `continue` branch lists its request once, and for_each items list theirs in item order. Red: the stacked, `after` item, skipped-need, sub-workflow and composite target tests failed with the second request opened into `main` (`+ "main"` where the needed step's branch was expected); "keeps a URL a stacked step reports …" failed with `expected … to not have property "data.dropped"` while the lookup still used the run base branch.
…vers
max_parallel bounds how many items have an agent at work. An item that
finished its agent and waits for its pull request has none, so it no longer
holds back the next item. An item that runs after another still waits for it
to succeed, so its target is that item's delivered branch.
Red: "starts the next for_each item while an earlier one delivers, since
max_parallel bounds agents" failed with `no lease: {"data":{"wait":{"reason":
"build[0] is delivering",…}}}`.
The gate marks a step as creating right before TeamAI asks the host to open
a request. When a cancel lands during the create and the process stops
before it hears back, the host holds a request the run never lists. The
drain in `next` now also looks up every step that settled otherwise with
that mark and no receipt for its pass, on an ended run too. It only looks
up and never opens one. A request it finds gets its receipt, and an ended
run queues its end record again to list it. An answer either way drops the
mark. A lookup that fails keeps the mark for a later call and is tried once
per drain.
Tests also pin what already held: retrying a failed delivery delivers again
in the same pass and attempt with no lease, retrying a protected-path
failure runs the step again in a new pass, cancelling a delivering step
keeps its branch on origin, a cancel during the lookup opens nothing, and a
request opened during a cancel is recorded with the step left cancelled.
Red: "records a pull request a cancelled delivery opened before its process
stopped, by looking it up on a later call, and opens no other" failed with
`expected undefined to deeply equal [ { …(5) } ]`. The later `next` left the
ended run without a receipt.
A run published while a step delivers carries what it delivers, the branch
its copy started from and the receipts so far. The install that claims it
resumes the delivery through the provider's lookup. When the first machine
already opened the request, the claimer records it and opens none. When none
exists yet, the claimer opens exactly one, into the delivered branch the
copy grew from. A snapshot whose receipt or reported URL is not one TeamAI
keeps is refused at the claim, naming the field.
No code change was needed: phase one's snapshot rules and the lineage fix
in the previous commits give this behaviour. These tests hold it.
Red: with the lineage fix reverse-applied, both resume tests failed at
publish (`expected { ok: false, error: { …(3) } } to match object { ok: true,
… }`). With the snapshot schemas parsing receipt and reported URLs as plain
strings, both URL refusal tests failed with the claim accepted (`expected {
ok: true, … } to match object { ok: false, … }`).
Scorers judge what the agent did, so delivery no longer decides whether, or
by which install, a pass is judged. A delivering pass is judgeable by the
install that reported it, as it will be once delivered. A pass whose only
failure is delivery_failed or protected_path counts as the agent's success,
also once a retry moved it into the step's history. A run claimed
mid-delivery is therefore judged by the reporting install. Before, its pass
was not judgeable there while delivering, and never on the claimer.
The evidence for such a pass reads the same agent pass. A label made while
the step delivered stays the pass's only judgement whether delivery then
succeeds or fails. Scoring stays pull-only: the engine never calls it, so
delivery never waits on a scorer.
The failure codes only delivery writes are now a type that `deliveryFailed`
and the delivery failure builder share, so scoring's list cannot drift from
what delivery writes.
Red: "judges the agent pass of a step that delivers, for the install that
reported it, whatever becomes of its delivery" failed with `expected [] to
deeply equal [ { subject: { …(3) }, …(1) } ]`. "judges a step while it
delivers, and its label stays the judgement however the delivery ends"
failed with no lease offered (ZodError: lease Required). After the scorer
fix it still failed: the entry closed as `evidence_empty` until the evidence
plan read the agent pass.
… branch Workflow delivery on a GitLab origin now finds the open merge request of a branch before it opens one, so a delivery that ran before, or ran on another machine, opens no second request. The lookup asks `/projects/:id/merge_requests` for the opened requests of the source branch into the target, with a timeout. It parses the answer at the boundary and keeps only a request from the project itself, not a fork with the same branch name. A lookup that cannot tell is a fault, never "none", since a false "none" opens a duplicate: no token, an HTTP error, an answer it cannot read, a full page without the branch, a URL on another host than the instance, or no answer in time. Its errors never repeat the host's text. Create already honours the delivery timeout. A test now pins that too. Red: both lookup tests failed with `gitlab has no pull request lookup`, and the origin test for `git@gitlab.com:acme/platform/app.git` failed with `expected false to deeply equal [ 'gitlab', 'acme/platform/app' ]`.
Delivery looks for an open request before it opens one, so a retry or a crash between create and receipt never opens a second. TGit had no lookup, so a git.woa.com origin delivered nothing. The lookup asks the v3 REST API for opened requests from the source branch into the target and keeps one from the same project, opened or reopened. It fails rather than answering none when TGit cannot be asked or answers in time, answers what TeamAI cannot read, fills a whole page without the branch, or names a request on another host. Red: both TGit lookup tests failed with `tgit has no pull request lookup`, and the origin test for `https://git.woa.com/acme/app.git` failed with `expected false to deeply equal [ 'tgit', 'acme/app' ]`.
Delivery looks for an open request before it opens one, so a retry or a crash between create and receipt never opens a second. GitCode had no lookup, so a gitcode.com origin delivered nothing. The lookup asks the v5 REST API for open requests from the source branch into the target and keeps one whose head and base are this repository. It fails rather than answering none when GitCode cannot be asked or answers in time, answers what TeamAI cannot read (including a state outside open, closed and merged), fills a whole page without the branch, or names a request on another host. Red: both GitCode lookup tests failed with `gitcode has no pull request lookup`, and the origin test for `https://gitcode.com/acme/app.git` failed with `expected false to deeply equal [ 'gitcode', 'acme/app' ]`.
Delivery must find an open request before it opens one, or a retry opens a second. Nothing in this code or its docs confirms how the cnb CLI lists open pull requests, and even the post-pull output it parses on create is unconfirmed, so a CNB lookup could answer a false "none". CNB keeps no lookup, the comment now says why, and a test pins that its origin is reached through git alone and opens no pull request. Red: none; the CNB origin test passed on arrival, because this commit pins existing behaviour and changes no code path.
f6490bc to
d609e16
Compare
|
Addressed the review follow-up at
The PR description contains the updated verification record. Ready for another review. |
Earlier cancellation and implicit-read-only command findings are resolved. The CI recipe’s missing runner is also resolved in both language versions. The description documents sufficient build-first real-CLI testing. Older commit references and deferred live-provider coverage are not blocking. Only the scoped diff was read; no PR code was run, built, or installed. |
The rebased history and its handover fix target the earlier design (deliveryDeadline, copies[].origin), which this branch replaces; the handover of delivering steps, receipts and pending creates is covered by its own snapshot rules and tests.
All earlier findings are resolved in the scoped diff: cancellation rechecking, implicit-read-only command and top-level prompt rejection, CI runners, and discarded leases. The description supplies sufficient build-first real-CLI verification. Older commit references and deferred live-provider coverage are not blocking. Reviewed only |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
@nateEc Thanks for this work. We are taking over #1303, #1304 and #1306 to speed up delivery. We will push the remaining changes to your branches, bring them up to date with main, and keep you as co-author in the squash commit. #1303 and #1306 go first, and #1304 follows once both have merged.
Please hold off on pushing to these branches in the meantime. All three stay blocked until we finish.
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
@nateEc Thanks for this work. We are taking over these three PRs to speed up delivery:
We will push the remaining changes to your branches, bring them up to date with main, and keep you as co-author in the squash commit. #1303 and #1306 go first, and #1304 follows once both have merged.
Please hold off on pushing to these branches in the meantime. All three stay blocked until we finish.
|
I have now seen the takeover reviews and will hold off on further pushes to #1303, #1304 and #1306. My previous updates to #1303/#1304 were pushed before I read that request; I will not overwrite your branch work. Thank you for taking these forward. For #1306, the two newest shared-branch findings reproduce on efdaf68. I prepared an optional, separate handoff commit without updating this PR branch: nateEc@b5180eea . It rechecks origin after lookup and the recorded head at the create gate, and retains pending delivery while a writer lease is active. All three added regressions fail before the change; 225 related tests, 4 built-CLI delivery E2E cases, build, types and lint pass. Please take or adapt it as useful; I will leave these branches to you. |
A sibling `continue` step can push, and report, a commit while delivery looks up the branch's open request. The create gate only checked that the step still delivered the same key, so the create went ahead with content no check had read. deliverPr now reads origin's tip again after the lookup, and the gate also refuses when `copies[branch].commit` is no longer the job's `recordedHead`. Either answers `busy`: nothing is listed or opened, the step stays delivering, and the next drain checks the new tip. Red: "opens nothing when a sibling ... and checks that commit on the next call" failed in all three cases with the create already made and `work.impl` delivered (`done: succeeded`, a wait only for `work.docs`, or the `work.docs` lease handed back) instead of `wait.delivering: ['work.impl']`.
…anch runs A step can push before it reports. Three `next` calls then failed the delivering step with `branch_moved` while the sibling's lease was still open, and its later report could not revive the delivery. A drain that finds origin's branch ahead of the recorded commit no longer counts toward the three-drain bound while another step that writes the same branch is leased. Red: "keeps waiting for a report of a commit ahead on origin while a step that writes the branch still runs, however many calls come" failed on the third `next`, which answered `wait.reason: "work.docs is leased"` without `delivering: ['work.impl']`, because the bound had failed `work.impl`.
… still at the checked commit A sibling can push while the lookup runs, and the lookup can find the branch's open request. Delivery then succeeded on the commit it checked before the lookup, though the request already held the sibling's unchecked one. The second read of origin's tip now comes before a found request is taken, as it does before a create. Red: 'opens nothing when a sibling pushes and records a protected commit while the lookup finds the branch's open pull request' failed with the run done instead of a wait for the delivery. Co-authored-by: Nathan Shan <nathanielcrush51@gmail.com>
All earlier findings are resolved: cancellation rechecking, read-only command and prompt rejection, CI runners, discarded leases, recorded-tip revalidation, and waiting for active shared-branch writers. The description documents sufficient testing, including build-first real-CLI verification at Only the scoped diff was read; no PR code was run, built, or installed. |
…e wrote A delivering workflow step with workspace: new delivers its own copy. A step inside it with workspace: new writes a branch of its own, and once another step recorded a commit on the copy, that branch was never part of the pull request, yet the workflow step succeeded. The job now carries the head of each step inside the delivering one that succeeded on another branch that no step inside it delivers. deliverPr requires each to be an ancestor of origin's tip, and otherwise fails with child_work_left_out, naming the step and its branch. A later step that merges that branch through needs makes the delivery pass. The run keeps the workflow it started with, so the remedy is a changed workflow and a new run. Red: "opens no pull request for a workflow step whose branch leaves out the work of a step inside it on another branch" failed with outcome "succeeded" instead of "failed".
…checked A URL a driver reported on the origin's own host was kept when the lookup that checks it failed or answered a URL TeamAI cannot keep, so an unrelated or nonexistent pull request URL stayed on the step and was delivered as it was. On the origin's host, such a lookup now drops the URL with the reason "could not be checked against the open pull requests of branch …", which never names the URL. Delivery's own lookup then finds the open pull request. A URL on another host is still kept as it is. The driver conformance case for an unreachable host now expects a reporting driver to fail delivery as the others do. Red: "drops a URL on the origin host when the lookup that checks it cannot answer" and "... answers with a URL on another host" failed with no dropped entry: the URL was kept.
…p accept later_protected_path told the person to have a step on the branch take the changes out. In the topology it exists for, the step that pushed them already succeeded, so no such step is left, and a hand-pushed removal makes origin's tip an ahead tip, which delivery rejects too. The message now names the two remedies the run accepts: put origin's branch back at the step's own commit and retry the step, which delivers it without the later commits, or retry the workflow step whose copy the branch is, which runs its steps again in a new branch. The job carries that workflow step as openedBy. branch_ahead no longer reuses the branch_moved text, which said the tip no longer holds the checked changes. Its own message says the tip holds the recorded commit and later commits no report recorded. Red: "names remedies for a protected change a later report recorded on the shared branch" failed with the old "Have a step on that branch take those changes out" message, and "opens and accepts nothing when origin's branch moved past the checked commit" failed with the branch_moved text.
A tip ahead of the recorded commit kept its step delivering for three drains in a row, counted in the step's persisted unrecordedTip. Since a writer lease already holds the wait, the count only waited on driver polls during which nothing could record the tip. completeDelivery now keeps the step delivering while a step that writes the branch is leased, or when copies[branch].commit is no longer the job's recordedHead, which a report that landed while the drain read the branch changes. Otherwise it fails the step at once. unrecordedTip, its schema member, snapshot rule and destructurings, AHEAD_DRAINS and awaitsReport are gone. The per-drain waiting list stays. Red: "opens and accepts nothing when origin's branch moved past the checked commit" (both cases) and "names the commit the run recorded last when the shared branch on origin is moved at a retry" failed with a wait answer instead of the failed run.
…leased The gate opened a pull request, and a reported URL was accepted, while a sibling that writes the same branch was still leased. That sibling's next push then entered the pull request with no delivery checking it. markCreating now answers busy while branchWriterLeased is true. The job carries writerLeased, read under the run lock when it is picked, and a reported URL or a request the lookup found is busy rather than accepted while it is set. Busy leaves the step delivering, so the drain after the sibling reports checks the sibling's commit. Red: the three cases of "delivers nothing while a sibling that writes the shared branch is leased" failed with wait.reason "work.docs is leased" and no delivering list: the step had already delivered.
Delivery bounds and silences its ls-remote, but the fetch it runs when the checked tip or the recorded commit is missing locally had neither a timeout nor a non-interactive environment. A stalled connection after ls-remote held the delivery lock, and leaseNext with it, without limit. fetchMissingCommits takes an optional bound and environment for its fetch. Delivery passes DELIVERY_TIMEOUT_MS and NON_INTERACTIVE_GIT_ENV, as it does for ls-remote. The local driver's callers are unchanged. Red: "fetches the commits it checks within the delivery timeout and without credential prompts" failed because fetchMissingCommits was called with no options, and the fetch's git process got the GIT_ASKPASS of the member's environment.
…eased nextDelivery picked a delivering step whose branch a leased sibling writes, and deliverPr read the job's writerLeased flag only after the tip checks and the provider lookup had run, to answer busy. So every next made a provider lookup while the sibling ran: with the local driver polling every 2 s, about 1800 gh api calls an hour. nextDelivery now passes over such a step before any I/O. The job no longer carries writerLeased, and deliverPr no longer reads it. The gate in markCreating keeps its check, which covers a lease granted while a delivery runs, and completeDelivery keeps its ahead rule. The tests that leased the sibling before the pick now lease it after the pick, inside the lookup or the git read, so each keeps its race: the four cases of "opens nothing when a sibling ...", and "keeps a step delivering when a report records origin's ahead commit ...". A sibling that has not reported is handed back to the driver, which did not list it as running. "keeps a step delivering while origin's branch is ahead ..." moves the same way, so it still covers the leased half of the ahead rule. A new test covers the gate's check for a sibling leased during the lookup that has not pushed yet. Red: "asks the host nothing while a sibling that writes the shared branch is leased, however many calls come" failed with three gh api lookups where none was expected.
…1306 Conflicts, resolved so both features hold: - step-state.ts: RunningSchema takes Tencent#1303's expiresAt and limitsFrom; DeliveringSchema and the shared variant list stay. CancelledSchema keeps settledFields, which now carry Tencent#1303's unenforced. cancel() keeps a delivering step's settlement and uses Tencent#1303's stoppedLease for a leased one. stopWhere, which Tencent#1303 moved here, cancels a delivering step as before. - engine.ts: this branch's stopWhere is dropped for Tencent#1303's. The drain, the gate and the report vetting take the run's secrets and reconcile with a time, as updateRun, queueRunRecord and reconcileRun now require. leaseNext resolves the env before the drain, so its writes mask the same secrets. parseReport reads unenforced and still names unread changes fields. A report marks its copy used and may start a delivery. - types.ts, snapshot.ts: copies hold from and used; deliveries, creating, releasedTokens, stoppedAttempts, limitsFrom, unenforced and expiresAt each keep a field rule. - conformance.ts, conformance-interpreter.ts and both runners: the runner moved into Tencent#1303's interpreter. Scenarios send base instead of workspace: true, since every profile can take workspaces, and the interpreter fills a report's changes from its saved lease. - parse.ts, records.ts, run-controls.ts, workspace.ts, the handoff and engine tests and docs/dev/workflow-engine.md keep both sides. - Tests call startRun, cancelRun, retryStep and updateRun with secrets, and pass workspace modes in can.
Timeouts and budgets now settle in reconcileRun, which both the delivery pick and the create gate apply. A deadline that passes while a step delivers leaves no lookup and no create; one that passes during the lookup leaves no create. The dev doc states where limits act. Red: with the gate ignoring a settled outcome, 'opens nothing once a timeout passes while its delivery looks for an open pull request' failed with createPullRequest called once; with the pick not reconciling, 'opens nothing once a timeout passes while its step delivers' failed with the lookup called once.
Earlier findings are resolved for their reported scenarios. The description documents sufficient build-first real-CLI testing at Only the scoped diff was read; no PR code was run, built, or installed. |
…se child work A workflow step with deliver: pr whose own branch holds nothing beyond the run base delivers nothing, so the child-work check never ran. A step inside it that wrote a branch of its own then succeeded with its work in no pull request. The step now fails with the child_work_left_out message when such work exists. Red: 'fails a workflow step whose own branch holds nothing beyond the run base while a step inside it wrote another branch' ended the run succeeded.
The create gate answers busy while a writer of the branch is leased, but a pull request the lookup found, or a driver reported, skipped the gate and settled the step, releasing its dependents before the writer reported. completeDelivery now applies the conditions it already used for an ahead tip: while a writer is leased, or a report recorded another commit since the pick, the step stays delivering and a request the job did not open gets no receipt. One it opened still gets its receipt. Red: 'takes no open pull request when a sibling that writes the shared branch is leased while the lookup finds it' found the step succeeded.
|
No new confident findings in All earlier findings are resolved in the current diff, including the unchanged-composite child-work check and shared-branch writer validation when reusing a PR. The description documents sufficient testing: build-first real-CLI verification at Only the scoped diff and trusted base review criteria were read. No PR code was run, built, or installed. |
SaulMoro
left a comment
SaulMoro
left a comment
There was a problem hiding this comment.
The open points are closed on this head: CI is green, the delivery driver profiles and the real-CLI E2E pass, and the latest review has no findings.
|
Thanks for the groundwork and the handoff in b5180ee. To speed up delivery we closed the remaining open points on your branch and merged it:
One note for #1304: its |
Summary
Closes #1090.
A workflow step that writes in a workspace can set
deliver: pr. When its agent succeeds, the step becomesdelivering, and TeamAI opens one pull request from the step's pushed branch, or reuses the open one. TeamAI never merges. Dependents wait until the delivery finishes. When a delivery fails, the branch stays on origin, and the error message names the branch and gives the retry command.Type of Change
Design notes
Each decision, with its reason:
deliveringis a step status. It keeps the agent's settlement and adds a delivery handle. It counts as neither stopped nor settled, so dependents wait.stopped,settled,takeOver,stopWhere,retryInstance) and theinProgress/cancellablepredicates place it throughneverchecks.pr-delivery.tsowns all I/O and imports nothing from the engine.delivery-ledger.tsholds the run's receipt and pending-create state, with one schema for each.next, whichever driver calls it. It runs right after the optional claim and before the locked body.wait, and itsnextcount stays bounded. The local driver polls onwait.delivering.GitProvider.pullRequests: { findOpen } | null, and create stayscreatePullRequest.findOpenfails closed: an error or an unreadable answer is a fault, never "no PR", because a false "none" creates duplicates.pr-delivery.ts.null, because its open-request listing cannot be confirmed from its CLI or docs, and generic git isnull.run.deliveriesis an append-only receipt list, deduplicated by URL. The end record'sprsis that list in order of first appearance, as [feat] Deliver workflow branches as pull requests #1090 asks. Under sequentialfor_eachitems, that is also item order.run.creatinglists each create TeamAI asked for and holds no receipt for. It lives on the run, not the step, so retry, cancel or archive cannot lose it.ls-remoteunder the repository's non-interactive git env, must be one of two commits:branch_moved.branch_missing, and a rewritten one withbranch_rewritten. Each message names the commit to restore.delivery_failed, naming that commit, rather than re-running this step's agent.AGENT_PATHS, the same list the workspace overlay uses.copies[].fromis written once, at the copy's first lease, from the orderplanWorkspacealready uses:after, thenneeds, then inherited needs.workflow start. So is adeliver: prstep, or a workflow step, that nothing in its tree writes. That second refusal also closes the earlier review finding about a top-level prompt with no writable workspace.newstep opens one PR per pass, on purpose. Each pass has its own/p<pass>branch. Earlier PRs stay open for a person to close; closing them is not TeamAI's call. Acontinueloop stays on one branch and lists its PR once.delivery_failedretries the delivery in the same pass and attempt, without running the agent again.protected_pathre-runs the agent in a new pass, since the same diff can never pass.ChangesSchemagains nopr_url. This departs from the 20-10 note. The wire field is exactlychanges.pr_url, and unknownchanges.*keys are listed inignored.run.deliveries, not in the step'schanges. Storedchangesstay git facts that every workspace reader trusts.https, has no credentials, and is at most 512 characters; its query and fragment are removed.droppedwith a reason and never its value.no-provider-merge.test.tsscanssrc/providers/**for merge calls, and a type test pins the lookup interface's keys.reconcileRun. Timeouts and budgets settle there, and both the delivery pick and the create gate apply it under the lock. A timeout that passes while a step delivers leaves no lookup and no create; one that passes during the lookup leaves no create. Two tests pin both orders, and the dev doc states the rule. The contributor'sdeliveryDeadlineis gone.busy, so a long writer does not cost one provider lookup pernext. A request the lookup finds or a driver reports gets the same check as a create: while a writer is leased, or a report recorded another commit since the pick, the step staysdeliveringand the request gets no receipt. One TeamAI opened keeps its receipt.child_work_left_outmessage instead of succeeding.runs-onin the CI recipe was a misread. Both guides already had it.workflow-docs-examples.test.tsnow checks that every recipe job hasruns-on, and that both languages show the same commands.Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses: 546 files, 15,755 tests ata8f6b6bd8; the delivery suite (109 tests) again atff5de032fReal CLI, at
ff5de032f(afternpm run build):npm run test:e2e -- workflow-pr-delivery-1090 workflow-delivery-drivers workflow-conformancegaveTest Files 3 passed (3),Tests 97 passed (97).workflow-pr-delivery-1090. It takes the CI recipe out ofdocs/guide/workflow.mdas written, substitutes its expressions, and runs everyrun:line against a fixture repository with a bare origin and a fakegh.jqquery finds that run in the published records.hitl_required, and nothing opens.workflow-delivery-drivers. It runs four kinds of driver through the built CLI's JSON only: the local driver; a session driver that commits and pushes from its own worktree; an external driver that knows nothing of delivery and sleeps on everywait; and a remote driver that opens its own PR from another clone and reports it.Each guard this PR adds was reverted once locally to confirm its test goes red, then restored. That covers 27 guards plus the review-round fixes, the no-merge scan included.
Not run: a live provider account (every host is faked or reached through fixtures), Windows, and agents other than command steps. Those are left to CI and maintainers.
This branch includes
mainwith #1303 merged. A note for #1304: its newskill-data/workflow/SKILL.mdshould treat awaitthat listsdeliveringas "callnextagain".Related Issues
Closes #1090. Part of #818. Follows #1086 and #1087.
Notes for Reviewers
deliver: pris opt-in per step. Without it, no run changes behaviour, and stored runs from before this change still parse.deliver: pr, and everynext, since each one now starts with a drain. A drain with nothing to deliver costs one run read under the lock.createPullRequestpaths (GitHub, TGit and CNB create) still spawn their CLI synchronously. Inside the local driver, a slow create can therefore stall the loop for up to its 30 s timeout. The new GitHub lookup is asynchronous.