A real Go regression, two Shipmoor tools: reviewing Alertmanager PR #2689

A small condition can invalidate the promise of an entire pull request.

Prometheus Alertmanager PR #2689 added optional callbacks to periodic maintenance. The intended behavior was simple: downstream users could inject custom maintenance logic, while Alertmanager itself would keep its existing garbage collection and snapshot behavior when no callback was supplied.

The pull request was reviewed, approved, and merged. It also introduced a crash on that default path. A later maintainer summarized the outcome plainly: “This introduced a crash bug for the default silence maintenance function,” and the fix landed in PR #2701.

This is a useful teaching case because the defect is not obscure. It is a three-line condition whose meaning depends on a promise made elsewhere: the callback is optional. Shipmoor Code Review can find the code defect. Shipmoor Claim Check can test whether the complete change still satisfies that promise.

This walkthrough uses the public PR range:

base  d6e758ad3792ce6a78032f6d2ae706f367422e35
head  58c5c9d983a61a286063e4f138ed55cf9c8297b0

The bug

The new silence-maintenance implementation creates a valid built-in function, then tries to replace it when an override is present:

doMaintenance := func() (int64, error) {
    // existing garbage collection and snapshot behavior
}

if doMaintenance != nil {
    doMaintenance = override
}

The guard checks the wrong value. doMaintenance was just initialized, so it is always non-nil. The assignment therefore runs even when override is nil.

Alertmanager’s production call site passes nil because the callback is optional. On the next maintenance tick or shutdown flush, the program calls a nil function and panics.

The repair is equally small:

if override != nil {
    doMaintenance = override
}

The important part of review is not how many lines a fix takes. It is whether the tool can connect the changed condition, the callers, and the intended default behavior without inventing unrelated advice.

Reproduce the change surface

Clone Alertmanager and fetch the historical pull request:

git clone https://github.com/prometheus/alertmanager.git
cd alertmanager
git fetch origin pull/2689/head:pr-2689
git checkout pr-2689

BASE=d6e758ad3792ce6a78032f6d2ae706f367422e35
HEAD=58c5c9d983a61a286063e4f138ed55cf9c8297b0

The range changes five files. Shipmoor reviews the three changed Go implementation files and treats tests and other selected files according to the repository’s review rules.

Step 1: run advisory Code Review

Run Review with the coding-agent CLI already authenticated on your machine:

shipmoor review . \
  --agent cursor \
  --from "$BASE" \
  --to "$HEAD" \
  --json \
  --output .shipmoor/alertmanager-review.json \
  --trace-out .shipmoor/alertmanager-review.trace.json \
  --verbose

--agent cursor is a Shipmoor preset. Shipmoor keeps Cursor in read-only ask mode, adapts its output to the review protocol, and uses the local provider relationship you already have. Shipmoor does not host the model or open a second model network connection.

The observed run produced one finding, at the causal repair location:

silence/silence.go:381-383  critical  high confidence

This condition is inverted: doMaintenance is always non-nil here, so assigning
override unconditionally makes the default path set doMaintenance to nil whenever
callers pass nil. The next ticker or shutdown flush dereferences a nil function and
panics. Check override != nil before replacing the default maintenance operation.

Two details matter:

• Review did not report the two production call sites as separate defects. Passing nil is a valid use of an optional interface; the repair belongs in the changed implementation.
• Review is advisory. A critical finding is still a review comment, not an automatic merge gate. The JSON and SARIF contracts remain advisory too.

The trace is for local diagnostics. It records phase status, timing, provenance, bounded metadata, and content digests. It excludes prompts, diffs, finding text, source evidence, secrets, and absolute paths.

Step 2: turn the pull-request promise into acceptance criteria

Review asks, “What is wrong in this diff?” Claim Check asks a different question: “Did this change satisfy the obligations we approved?”

Create .shipmoor/acceptance.yaml:

goal: Enable optional custom maintenance callbacks without changing Alertmanager's default maintenance behavior
items:
  - id: AC-01
    class: judgment
    statement: When no custom callback is supplied, silence and notification-log maintenance keep using their built-in garbage-collection and snapshot behavior without calling a nil function
    source_span: "without changing Alertmanager's default maintenance behavior"
    required: true

  - id: AC-02
    class: judgment
    statement: When a non-nil custom callback is supplied, periodic and shutdown maintenance invoke that callback instead of the built-in maintenance operation
    source_span: "Enable optional custom maintenance callbacks"
    required: true

Commit and review this file in a real pull request. A committed acceptance set is a human-approved contract; model output does not get to approve its own questions.

For the historical reproduction, put the PR intent in pr-intent.md:

Enable support for optional custom callbacks during periodic silence and
notification-log maintenance. The change should be a no-op for Alertmanager's
existing behavior when no custom callback is supplied, while allowing downstream
implementations to inject custom maintenance logic.

Step 3: run the deterministic floor first

Start without a model:

shipmoor claim-check . \
  --floor-only \
  --from "$BASE" \
  --to "$HEAD" \
  --acceptance .shipmoor/acceptance.yaml \
  --intent-prompt pr-intent.md \
  --vsa-out .shipmoor/alertmanager-floor.vsa.json \
  --trace-out .shipmoor/alertmanager-floor.trace.json \
  --verbose

The expected result for these two judgment obligations is INCONCLUSIVE. That is not a failed model call: no model was requested. The floor can verify deterministic evidence, but it cannot silently claim that a judgment obligation passed.

In the observed reproduction, the floor reported:

Claim Check  INCONCLUSIVE
verified 0 | divergent 0 | judge-open 2 | gaps 0
scan green | review not checked

This first run checks that the change range and acceptance contract load correctly before you spend any provider calls.

Step 4: run full Claim Check

Now let the same read-only agent drive binding, review, and judgment:

shipmoor claim-check . \
  --agent cursor \
  --from "$BASE" \
  --to "$HEAD" \
  --acceptance .shipmoor/acceptance.yaml \
  --intent-prompt pr-intent.md \
  --vsa-out .shipmoor/alertmanager-claim-check.vsa.json \
  --trace-out .shipmoor/alertmanager-claim-check.trace.json \
  --verbose

The observed full run returned BLOCKED:

Claim Check  BLOCKED | coverage 100%
verified 0 | divergent 1 | judge-open 1 | gaps 0
review red | review+scan 1 must-fix

It carried three independent reasons with resolving evidence:

1. The Review producer proposed and validated one must-fix at silence/silence.go:381.
2. The bound check for AC-02 did not verify the required callback behavior.
3. AC-01 reached 3/3 judge agreement on the default-path divergence and pointed to silence/silence.go.

The deterministic scan was green. That did not erase the review or acceptance evidence. Green static rules and a broken behavioral promise can coexist.

Build and test runners were not configured in this reproduction, so Claim Check said build not checked and tests not checked. It did not turn missing evidence into a pass. In a production repository, configure those runners so the final verdict also includes the real build and test results.

Review and Claim Check are useful for different reasons

The two commands found the same repair, but they answered different questions.

Tool	Primary question	Result in this case
Code Review	Is there a confirmed defect in the changed code?	One causal finding at silence.go:381
Claim Check	Does the change satisfy the approved obligations?	BLOCKED, with 3/3 agreement and witnesses

Use Review while shaping a change and discussing code. Use Claim Check when a requirement must remain explicit, independently evidenced, and attached to a merge decision.

Claim Check does not make Review blocking by severity. Only a narrow, validated must-fix proposal can enter the gate, and the acceptance judgment still has its own evidence and agreement rules.

After the fix

Apply the override != nil guard, add a regression test for nil/default maintenance, run the repository’s build and test commands, then rerun both Shipmoor commands.

Do not assume the next verdict will be READY merely because the condition changed. Claim Check should say READY only when every required obligation is accounted for and every configured producer supplies trustworthy evidence.

That is the point of the workflow: one review comment can locate the defect, while a frozen acceptance contract keeps the product promise visible until the evidence actually supports it.

Try the workflow on your own pull request

Start free, authorize the installation, then run:

shipmoor review . --agent cursor --from origin/main --to HEAD
shipmoor claim-check . --agent cursor --from origin/main --to HEAD

• Learn how Code Review works

• Learn how Claim Check works

• Code Review product page

• Claim Check product page

• From the shipmoor.dev Engineering Team