Skip to content

[safety] Nothing decides whether an agent may merge to main — three unattended merges already happened #314

Closed

Description

@serge-ivo

Observed

Run 73ffc073 on the FWS Repo Coder, from a single delegated objective, unattended:

  1. Uncommitted work — committed to branch feat/uncommitted-work, PR Agent work as first-class Tickets — granular, with reasoning + per-ticket conversation #150 opened and merged to main.
  2. Issue CODER-007 — (Stretch) Real-time status push instead of polling (Durable Object / relay) #83 — … PR Existing instances stuck on a non-tool-capable model — #100 fix only covers NEW agents, and there's no way to change an instance's model #151 opened and merged.
  3. Issue CODER-006 — (Optional) Aggregate live session-status endpoint (one call for all sessions) #82 — … PR Voice: unmute command word — dictation-based wake-to-record with configurable keyword and start-recording signal #152 opened and merged. Issue closed.
  4. All open bug issues exhausted — gh issue list --label bug --state open returns [].

Three merges to main of a real repository, plus an issue closed, with no human in the loop. The
objective did say "merge each before starting the next" — so the agent did what it was told. That
is the point: nothing decided whether it was allowed to.

The gap

There is no notion of merge authority anywhere:

Spend is recoverable. A merge to main is not, and neither is a closed issue.

What this is not

Not an argument that autonomous merge is wrong. For a solo operator dogfooding their own repos it
may be exactly what is wanted, and forcing a review step on every trivial fix would make the Coder
much less useful. The problem is that it is neither chosen nor visible.

Proposal

  1. A declared merge policy per repo, defaulting to the safe end:
    none (commit only) · pr (open, never merge) · merge (current behaviour).
    Repo-level rather than agent-level, because the answer differs per repository — a dogfood repo
    and a customer repo are not the same risk.
  2. Surface it where work is reported. A run that merged should say so in its outcome and on the
    board card. Today "PR Agent work as first-class Tickets — granular, with reasoning + per-ticket conversation #150 opened and merged" is buried in a run detail string nobody reads
    unless they go looking.
  3. Consider the same treatment for other irreversible acts the engine can reach: force-push,
    branch deletion, closing issues, gh release. Merge is the one that has actually happened, so
    it is the one worth doing first — the rest can follow the same declaration.

Verification

  • With policy pr, a delegated goal instructing "merge it" opens the PR and reports plainly that
    merging is not permitted, rather than silently complying or silently refusing.
  • A run that merges records it in its outcome and on the board.
  • Default for an existing repo is the current behaviour or pr — decided deliberately, since
    changing it silently is its own surprise.

Activity

  1. serge-ivo commented on Aug 6, 2026

    @serge-ivo
    ContributorAuthor

    Two things found while reading the code, before anyone builds this

    1. This cannot be a hard control — say so, or someone will ship it believing it is

    The engine runs claude --dangerously-skip-permissions / codex exec --sandbox danger-full-access.
    gh pr merge is reachable from that shell, always. No string in a prompt changes that.

    So a merge policy is declared intent injected into the Pilot's instructions, plus detection after
    the fact
    — it makes the boundary explicit and per-repo instead of unstated, and it is the layer a
    real control would later enforce. It is not enforcement itself.

    Worth writing into the code and the UI copy, because the failure mode of overclaiming here is
    already on the board: #246 shipped a dry-run guard that inspected only click, while the prompt
    told the model "Submit clicks are blocked anyway" — a guarantee that was true of one action type
    and believed for all of them.

    Real enforcement, if it is ever wanted, is a different ticket: a wrapped git credential, a
    protected branch on the GitHub side, or a review gate the runner cannot bypass. Those are controls.
    This is a norm, and the two should not be confused in the copy.

    Corollary: detection is the half that actually holds. A run that merged must say so in its
    outcome and on the board card regardless of policy — that part does not depend on the model
    cooperating, and it is what turns "three merges happened and nobody knew" into something visible.

    2. The clause must give the model a correct action, not a prohibition

    A bare "do not merge" against an objective that says "commit, push, PR, and merge each one before
    starting the next"
    is a contradiction, and #254 is the recorded consequence of handing this
    model a contradiction: it obeyed the capitalised prohibition, denied work it had really done, and
    told the user to do it by hand.

    So the clause should read as a rule about what to do when the objective asks for more than the
    policy allows. Roughly:

    PULL REQUEST ONLY. Commit, push, and open a PR. Do not merge it. If the objective tells you to
    merge, open the PR anyway and then say plainly that merging is not permitted for this repo and the
    PR is waiting for review — that IS completing the objective as far as you are allowed. Do not
    treat it as a failure.

    That last sentence matters: without it a policy refusal looks like outcome: failed to the
    supervisor, and the Lead will report a successful run as a failure.

    Grounding for whoever picks this up

    • The Pilot's prompt is built by systemPrompt(goal) in lib/coding-loop.ts:172; CodingGoal
      (:16) is the carrier — it already threads specialInstructions and dryRun the same way.
    • coding_repos already has an instructions TEXT NOT NULL DEFAULT '' column (migration 0031)
      as precedent for per-repo prompt text, so a merge_policy column follows an established shape.
    • The repo PUT /:instanceId/coding/repos/:repoId route currently accepts only name and urls;
      it is the natural place to set the policy.

    On the default

    pr rather than merge, and note this does change behaviour for repos whose agents merge
    today — which is the point: the current behaviour was never chosen, it was the absence of a
    decision. A default of merge preserves the surprise and makes the setting decorative for everyone
    who never finds it.

    The costs are not symmetric: being wrong toward pr means a PR someone has to click; being wrong
    toward merge means an unreviewed commit on main.

  2. serge-ivo commented on Aug 6, 2026

    @serge-ivo
    ContributorAuthor

    Shipped in da7f381.

    Where the gate sits

    workers/api/src/lib/coding-authority.ts is the decision half; #294 was already the detection half. Policy per repo: merge (may merge to the trunk) · pr (may open a PR, never merge or push to main) · none (commit only).

    Resolution is repo override → agent setting → platform constant, each one value to change:

    • repo — PUT /v1/instances/{id}/coding/repos/{repoId} {"mergePolicy":"pr"}; "" clears it back to inherit. An unrecognised value is refused, not ignored — believing you tightened a policy you did not is worse than an error.
    • agent — a typed merge_policy settingsSchema field (migration 0091), added to every agent declaring the coding surface. It renders in the console's existing "Agent settings" card, so there was no UI to build.
    • platform — DEFAULT_MERGE_POLICY in coding-authority.ts.

    Three layers, and only two are enforcement:

    1. Prompt (coding-loop.ts systemPrompt) — the policy, worded as overriding the objective, since "merge each before starting the next" is what a human typed. Advisory.
    2. Instruction screen (runCodingLoop) — the Pilot's outgoing instruction is checked before it reaches the Engine. Deterministic, cloud-side, and the layer that would have stopped 73ffc073. The refusal is fed back through the step log the loop already shows the brain, so it adapts; capped at 3 refusals so a heuristic false positive costs a stopped run with a clear reason rather than a silent spin; and announced in your chat thread, because silently refusing is the other failure mode this issue names.
    3. Observed act (coding-session.ts capture + closing drain) — an act the policy forbids halts the run, writes act.unauthorized to the trace at level: error, and raises a needs_human board card. Checked in the closing drain too, because a coding run routinely ends with the merge — the incident's own shape.

    Reporting (item 2 of the proposal): the delegated run's detail, its board card and the session timeline all carry the policy line whenever one is in force, with any breach named ahead of the ordinary Acts: summary — buried after "and 3 more" it would be the same invisibility this issue is about.

    What it does NOT do — please read this part

    • It is not a kernel-level block. The Engine runs --dangerously-skip-permissions on your machine, so gh pr merge stays physically available to it. Layer 3 catches the merge after it happened and stops the run before the second. It bounds three merges to one; it cannot make the first not happen. Denying the command at launch (--disallowedTools) lives in the runner and is follow-up work.
    • Only a stream-json engine reports acts. A raw Codex/Grok session emits nothing parseable, so layer 3 sees nothing there — a restrictive policy is layers 1 and 2 only. Rather than let that look like protection, the run note says it out loud: "the codex engine does not report what it ran, so this run's actual commands could not be checked."
    • pr does not forbid push.force. Force-pushing a feature branch is ordinary in a PR workflow and a policy that broke rebasing would be switched off within a day. A force-push aimed at the trunk classifies as push.force, not push.trunk, so it is a known narrow hole.
    • Merge only. Force-push, branch deletion, closing issues and gh release follow the same declaration when someone wants them — none already covers push.force/branch.delete, and issue-closing has no act kind yet.
    • No repo-level UI. The agent-wide setting renders itself; the per-repo override is API-only this round (the console was owned by another change).

    The default is yours to choose

    I shipped merge — today's behaviour, unchanged byte for byte. Every arm of the gate is a literal no-op under it: no prompt text, no screening, no forbidden acts, and describeAuthority returns null so no run claims protection it does not have. There is a test whose only job is to assert exactly that.

    That was deliberate, not a preference for permissiveness. Flipping the default to pr would change how your Coder behaves on repos you never touched — work you rely on would start failing without you asking for it, which is the same unannounced-change problem this ticket is about, pointed the other way.

    My recommendation: set pr as the agent-wide setting, and leave merge on the dogfood repos you actually want merging unattended. That inverts the risk correctly — the repos where autonomous merge is fine are the ones you can name, and everything you add later is safe by default. One field on the Coder's settings does it.

    CI status — stated plainly

    No CI run verified this. GitHub Actions is in a declared major outage and there were 24 unverified commits on main when I started. Verified locally against the rebased tree:

    • pnpm typecheck — clean (11 projects)
    • npx vitest run — 293 files / 4498 tests passing
    • npx biome check — 14 errors / 283 warnings, identical to the pre-change baseline; 0 on files I touched
    • pnpm docs:drift — no drift (API coverage 282/282)
    • node scripts/check-file-size.mjs — OK; routes/coding.ts pin raised 1769 → 1774 on purpose, reason recorded in the PINS comment and the commit
    • migration 0091 applied to a scratch SQLite fixture: appends to existing schemas, leaves non-coding agents alone, survives a legacy config = '' row, idempotent on re-run
  3. serge-ivo commented on Aug 6, 2026

    @serge-ivo
    ContributorAuthor

    Closing: the mechanism is in and the setting is exposed. The remaining items (per-repo UI, runner-level command denial, the same declaration for force-push/branch-delete/issue-close/release) are follow-ups rather than parts of this, and are listed in the comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions