Repository navigation
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.
`MODEL_AUTH` selects which credential the worker authenticates with: `sdk` reads CLAUDE_CODE_OAUTH_TOKEN, `api-key` reads ANTHROPIC_API_KEY. It is explicit configuration and never inference, because both credentials look alike to the SDK and bill completely differently — letting whichever happened to be present in the environment decide would make the billed party a property of the pod rather than of a decision, and the failure is silent: a run that quietly spends someone's personal quota looks exactly like a working one. One mode's credential is never substituted for the other; the worker refuses to start and names the one it found. `sdk` is the default, by operator decision. Note the flip side: a deployment carrying only ANTHROPIC_API_KEY now refuses to start rather than using it. `ModelCredential` is tagged with its mode rather than passed as a bare string, because the two are injected into the model's subprocess under *different* variable names and getting that wrong does not fail loudly — the SDK simply finds no credential where it looked. Ported from feat/subscription-auth, which forked before the spend-ceiling, attempt-counting, pull-request and wiring slices and so could not run the pipeline end to end. src/agent/apiKey.ts becomes src/agent/credential.ts.
The chart had never been rendered. mclabels' values.schema.json only accepts
component values from a fixed enum, and ours was `worker`, which is not a
member, so every `helm template` and `helm lint` failed schema validation
before reaching the templates. This outbound-only poller is a backend process,
so `backend`.
With that fixed the new MODEL_AUTH block renders and picks the right branch in
both modes: api-key binds the Secret to ANTHROPIC_API_KEY, sdk binds the same
Secret to CLAUDE_CODE_OAUTH_TOKEN, each parsing as valid YAML at the correct
depth in the env list. The variable name is the mode's, never both at once, so
the chart cannot hand the worker a credential it will refuse.
Also strip trailing whitespace in deployment.yaml; the run after `{{- end }}`
on the extraEnvVars guard was leaking into rendered output as trailing spaces
on the secretKeyRef's `key:` line.
Folds in f93729a from chore/helm-mclabels, which was never pushed and sat on
the pre-wiring lineage. Adapted to this branch's value names: the credential
Secret is worker.anthropicSecretName / anthropicSecretKey, not modelSecretName.
Three changes to the configuration section: MODEL_AUTH and both credentials in the table, a "Which account pays for the model" subsection explaining why the mode is stated rather than inferred, and the dry-run recipe switched to sdk. The subsection carries the warning that code cannot: Anthropic's Agent SDK documentation states that claude.ai login and its rate limits may not be used for products built on the Agent SDK unless previously approved, and `sdk` is the default — so running the worker at all asserts that this deployment has that approval. Three consequences no code can fix are listed with it: the shared quota, the attribution, and the crash-loop when a token expires. Also replaces the stale known-gap note about `helm lint`, which said the mclabels enum fix "lives in the auth-modes branch rather than here" and told the reader to pass `--set mclabels.component=backend`. Both are now untrue, and the private-ACR steps the note glossed as "fails without registry access" are written out instead.
Running the dry run meant a nine-variable command line, or exporting secrets into the shell VS Code had to be launched from. Both now read `.env`, which .gitignore already covers: `npm run dry-run:local` through node's --env-file, and the debugger through the launch config's envFile. `.env.example` is the committable template and carries no values. `src/preflight.ts` is the part worth having. Since the queue now holds a real agent-ready ticket, the first thing a misconfigured run does is claim it — so there is no longer a free way to find out that a token is missing. Preflight checks both credentials and the worker config and touches nothing else: no Jira call, no model call, no writes. It lives in src/ beside dryRun.ts because that is what tsconfig covers and what makes the @src and @common aliases resolve. Also adds the "Integration: one cycle (fakes)" launch config, for stepping through the same runCycle seam with every edge faked and nothing spent.
The first real run refused every ticket with `no-transition`. The README had flagged the vocabulary as unverified and asked for the `offered` names from a real run; they came back as `Open to In-Progress`, `Open to Reject`, `Open to Verified`, `Canceled`, `Blocked` — and `jira_get_transitions` reports `id` and `name` only, with no target status on any entry. So both existing strategies missed: there was no `to` to match, and the name `Open to In-Progress` is not the status `In Progress`. A third strategy reads the target half out of a `<from> to <to>` name, and statuses now compare with case, hyphens, underscores and runs of whitespace flattened — `In-Progress` and `In Progress` are the same status and the hyphen was never meant to carry meaning. The strategies are split by whether an entry reports a target rather than tried in sequence over all of them, which is the part worth reading twice. A reported target is authoritative *for that entry*: a transition that says it lands in Blocked does not land in In Progress however it happens to be named, and letting the name win there would transition a ticket somewhere nobody asked for. Writing the wrong status is worse than refusing, so the name is consulted only where there is nothing better to go on. A test covers exactly that case; it failed against the first draft of this fix. `src/jira/mcpJira.ts` needed no change — `toTransition` already coerces the numeric id the server sends and already treats `to` as optional.
The claim half is verified now, so the known gap that asked for it is narrowed to what is still unknown: the set offered on an In Progress ticket, which is what a hand-back needs. That one matters more than the claim did — a release that cannot find its way back to Open deliberately keeps hold of the ticket, and the orphan sweep that would recover it does not exist yet. Also corrects two claims that are no longer true: `jira_get_transitions` is not rejected by the write pilot, and the lookup no longer matches on target status with the name as a fallback.
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.