Conversation
MAPCO-11434 and MAPCO-11433. `runCycle` claimed a ticket and handed it straight back; a fully tested implement-and-verify core sat next to it that nothing called. `handleTicket` now calls it, between the claim and the release, so labelling a ticket `agent-ready` runs the whole pipeline: resolve the repository from the title, clone it at its default branch, hand the ticket to the model inside that clone, run the repository's own tests, and on a passing run push an `agent/` branch and open a pull request. Two things had to exist first, and both were blockers rather than polish. `DescriptionPort` had no implementation, so the honest stub answered `''` for every ticket and `implementTicket` refused all of them before the first model turn — which is why labelling a ticket proved only that the claim and release plumbing worked. `createDescriptionReader` reads the prose per claimed ticket through `JiraPort.getIssue`, whose field list now asks for it. `POLL_FIELDS` deliberately still does not: the poll fetches one more ticket than it will work, so prose there is paid for on every tick for tickets nobody touches. An absent description is therefore a different fact from an empty one, and only the second is a refusal. Nothing produced the `workdir` that `implementTicket` and `CliGit` both take, so MAPCO-11433's clone criterion was unmet and no end-to-end run was possible. `CloneWorkspace` makes one shallow, single-branch clone per ticket in a `mkdtemp` directory and removes it in a `finally`, including on the publish failures that throw by design. The branch is GitHub's `default_branch` and not the remote's HEAD: the org has both `master` and `main`, and a checkout that disagreed with the pull request's base would diff against the wrong one. The credential reaches git through the environment and a helper, never argv, and only for `https://github.com` — the helper answers for whatever host git asks about, so an unvetted remote is a token handed to it. The spend ceilings now bite. `meterAgent` wraps the agent per ticket and charges every hand-off to the ledger, which is what makes `MAX_TOKENS_PER_TICKET` an in-flight bound rather than a retrospective one; charging once after `implementTicket` returned could only ever notice an overspend after the money was gone. Turns are charged from the run's own `num_turns`, now carried on `AgentRun`, because charging one per hand-off would have made a ceiling of forty turns mean forty hand-offs of forty turns apiece. Cache reads count, because they are billed prompt tokens and an agentic loop re-reads its context every turn. `cycle.runLine()` replaces the run line's hard-coded `tokensSpent: 0`. That leaves two give-up paths able to fire on one ticket — the implement loop's and the budget guard's — and an overspend takes both: the abort hands the ticket back, and the run it stopped is then reported as a give-up, which would comment again on a ticket already in Open that a human may have picked up. `createHandBackOnce` is a per-ticket latch that both bind to, so the first caller writes and every later one is told what it achieved. It stays open when the first attempt wrote nothing at all, because a silent held ticket is worse than a second try. `createImplementer` is two calls now. The agent must be built once at boot, where the model credential is read, so a pod with no Secret fails to come up rather than claiming a ticket to discover it; the hand-back belongs to one ticket, because the latch does. `src/worker.ts` is the one place those lifetimes are decided, and `src/index.ts` and `src/dryRun.ts` differ only in what they do with the result. `runCycle` also hands a held ticket back when something throws part-way. The existing paths keep hold of such a ticket on the argument that the boot-time orphan sweep recovers it, but that sweep is MAPCO-11432 and does not exist, so the ticket would stay assigned to a bot for ever. The chart gains the workspace `emptyDir`, the two credentials, the budget knobs and the commit identity, and resources sized for a pod that now runs `npm ci` and a test suite rather than one that polls Jira. The image gains `git`. Still open, and deliberately: the commit stages every changed path because `AgentRun` reports whether the tree changed and not what changed; only Node repositories can be verified, so MAPCO-11433's Python criterion is unmet; and the GitHub credential is a static `GITHUB_TOKEN` rather than the per-run App token MAPCO-11428 specifies — checked at boot, because discovering it per ticket would cost a claim, a comment and a counted attempt on each one.
|
🎫 Related Jira Issue: MAPCO-11434 |
MAPCO-11433 asks for a workspace cleaned up on every path, and one path did not. `create` made the directory with `mkdtemp` and only then minted the push credential, with both calls above the `try` whose `catch` removes the directory. `mint` throws on a missing `GITHUB_TOKEN`, so that throw left an `agent-ticket-*` directory behind with nothing holding a reference to it: `deliver` turns the failure into a `no-workspace` hand-back and never receives a `Workspace` to dispose. One leak per ticket fills the volume and then fails every ticket for a reason that looks like something else. The mint moves above `mkdtemp` rather than inside the `try`, because the `catch` redacts the failure message with the token and so needs it in scope first. It also keeps the invariant already stated at the top of `create` literally true — refused before the directory is made and before the token is minted — so a credential the worker cannot produce now costs nothing and leaves nothing, exactly as an unvetted remote already did. Two docstrings referred to a `deliverTicket` that does not exist; the module exports `createDelivery` and a `DeliveryPort.deliver`. `Assignment`'s was also wrong about the work: it credited one function with both the claim and the clone, where `handleTicket` claims and `deliver` resolves and clones.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #10 — review that first, and this PR retargets as the stack merges.
This closes existing acceptance criteria on MAPCO-11434 and MAPCO-11433 — it is not new scope. Both tickets are already
Waiting for PRwith the relevant criteria unmet. No new ticket was filed; if you would rather see the wiring tracked separately, it is a Task under MAPCO-10721, but the work below is 11433's and 11434's.What this is
runCycleclaimed a ticket and handed it straight back. Separately, a fully tested implement-and-verify core sat next to it that nothing called.handleTicketnow calls it, between the claim and the release — so labelling a ticketagent-readyruns the whole thing rather than proving the claim/release plumbing works.Poll → claim (within the day's allowance) → resolve the repo from the title → clone it → hand the ticket to the model inside the clone → run the repo's own tests → push an
agent/branch and open a PR → clean up. Anything that stops short is a hand-back: a comment, the attempt counter bumped, the ticket back in Open.Base branch
feat/branch-commit-pr(#10), because it is the only tip carryinghandBackTicket(#9), the budget guard (#8) andpublishPullRequest(#10) together, and bothReleasePortandAbortPortsay in as many words to bind tohandBackTicketrather than write a second release path.That means this does not include #13's model-credential rework. #13 replaces
apiKey.tswithcredential.tsand adds its own helm block for the auth modes; theANTHROPIC_API_KEYsecretKeyRefadded here is the #10-leg equivalent, so that block is the expected conflict when #13 rebases. Nothing incycle.tsorworker.tsdepends on which of the two is underneath — the credential seam is insidecreateSdkAgent.DescriptionPorthad no implementation. The honest stub answered'', andimplementTicketrefuses an empty description before the first model turn — so everyagent-readyticket was claimed, refused asno-descriptionand handed back. That is why labelling a ticket proved only the plumbing.createDescriptionReaderreads the prose per claimed ticket throughJiraPort.getIssue, whose field list now asks for it.POLL_FIELDSdeliberately still does not, which is a change from the handoff's suggestion: the poll asks for one more ticket than it will work, so a description there is paid for on every tick for tickets nobody touches — and that is theDescriptionPortdocstring's own argument, so putting it inPOLL_FIELDSwould have contradicted a docstring in the same PR. The consequence worth knowing: an absent description and an empty one are now different facts, and only the second is a refusal.Nothing produced the
workdir.implementTicketandCliGitboth take one and no clone code existed, so MAPCO-11433's criterion was unmet and no end-to-end run was possible.CloneWorkspacemakes one shallow single-branch clone per ticket in amkdtempdirectory and removes it in afinally— including on the publish failures that throw by design. Three things about it are deliberate:default_branch, not the remote's HEAD. The org has bothmasterandmain; a checkout that disagreed with the PR's base would diff against the wrong one.tests/unit/vcs/clone.spec.tsasserts this against a real bare repo whose HEAD ismainand whose work is onrelease— a fake that agreed with us about--branchwould prove nothing.CREDENTIAL_FROM_ENV. The shared plumbing is extracted tovcs/gitInvoke.tsso the clone and the push cannot drift apart;cliGit.ts's public surface is unchanged and its real-git scratch-repo spec passes untouched.https://github.comgets a credential. The helper answers for whatever host git asks about, so an unvetted remote is a live token handed to it.file:is allowed and deliberately credential-free.The spend ceilings now bite, and that needed two decisions
meterAgentwraps the agent per ticket and charges every hand-off to the ledger. Charging once afterimplementTicketreturns cannot work: by then the ticket is either already handed back or has a verified change waiting, so the ceiling could only be enforced in hindsight and one ticket could spend ten times it before anything noticed.num_turns, now carried onAgentRun.TicketLedger.charge's docstring warns about exactly this: charging1per hand-off would makeMAX_TURNS_PER_TICKET=40mean forty hand-offs of forty turns apiece. This is the one change to a merged slice's public type in this PR.MAX_TOKENS_PER_TICKET. They are billed prompt tokens and an agentic loop re-reads its whole context every turn; excluding them would put most of what a ticket costs outside the ceiling that exists to bound it.cycle.runLine()replaces the run line's hard-codedtokensSpent: 0, and the guard is opened per cycle insiderunCycle— so the spend resets per line while the daily counter does not. There is a test for exactly that, because the alternative was the documented defect.Wiring the two slices together creates a case neither could see alone. On an overspend,
chargeSpendcallsAbortPort.abort— which hands the ticket back — and the run it stopped is then reported toimplementTicketas a give-up, which hands it back again: a second comment and a second transition on a ticket already in Open that a human may have picked up.BudgetExhausted.alreadyStopped's docstring describes this hazard and cannot prevent it; its latch stops repeated charges, not a different code path releasing.createHandBackOnceis a per-ticket latch that both ports bind to. The first caller writes; every later one is told what the first achieved. The overspend note wins, which is the right way round — it says what the ticket cost and which ceiling it hit, where the give-up note would only say the run did not finish. It stays open when the first attempt wrote nothing at all (attemptCounted: falsemeanshandBackTicketrefused before its first write), because a silently held ticket is worse than a second try.Lifetimes
createImplementeris two calls now, because collapsing them was not viable: the agent must be built once at boot, where the model credential is read, so a pod with no Secret fails to come up rather than claiming a ticket to discover it — while the hand-back belongs to one ticket, because the latch does.src/worker.tsis the single place those lifetimes are decided and has a spec asserting them.runCyclealso now hands a held ticket back when something throws part-way. The existing paths keep hold of such a ticket on the argument that the boot-time orphan sweep recovers it — but that sweep is MAPCO-11432 and does not exist, so the ticket would stay assigned to a bot for ever. A ticket the worker never managed to claim is still never written to.Deployment
The image gains
git. The chart gains the workspaceemptyDir, the two credentials, the three budget knobs, the commit identity, and resources sized for a pod that now runsnpm ciand a test suite — the previous100m/128Miwas right for a Jira poller and would OOM-kill on the first install.Verification
37 files, 468 tests, all green; coverage 88.7% statements / 87.0% branches, up from 87.0/86.1.
prettier --checkandeslintclean.helm lintpasses with--set mclabels.component=backend. Without it, it fails onmclabels.component: worker, which is not a member of the subchart's enum — a pre-existing defect on this leg of the stack, fixed on #13 (fix: align the mclabels block with the org's service charts). I deliberately did not fix it here, so as not to duplicate that commit and conflict with it. The chart renders valid YAML both with and without the optional secrets.Two specs drive the real
gitbinary against real repositories, following the precedentscratchRepo.spec.tsset.Deliberately still open
AgentRunreports whether the tree changed, not what changed, sopublishPullRequestgetsUNREPORTED_WRITES— feat(MAPCO-11436): worker builds the branch, commit and PR in code #10's own designed-for state. Bounded (status --porcelainnever reports an ignored file) and stated in the PR body and the log, but a repo that does not gitignore its build output gets a noisier diff than a human would have committed. Forwarding the paths the SDK already carries is what removes the branch; it is not this PR's scope.pytest).TestRunneris a port so that is an implementation rather than a reshape, but it does not exist and a Python ticket is refused asnot-verifiable. MAPCO-11433 is therefore not fully closed by this PR — its clone and cleanup criteria are, its language-inference criterion is not.GITHUB_TOKENis a static token, not MAPCO-11428's per-run App token.vcs/envToken.tsexists to be deleted; everything around it already treats the credential as short-lived. It is checked at boot rather than per ticket, because a missing credential discovered per ticket costs a claim, a comment naming an env var, and a counted attempt on every ticket — and two of those retire a ticket from the queue.STATUS_NAMEShas noWaiting for PR, so none is attempted.Grep) that are usually glibc-linked. A failingGrepdegrades a session rather than breaking it — the model still hasGlobandRead— but it is worth checking in the first pod logs.