DEV Community: mohammed faizan mohiuddin

Smashing an "index out of range [-1]" crash in lazygit (80k⭐)

mohammed faizan mohiuddin — Mon, 27 Jul 2026 10:32:45 +0000

This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.

Project Overview

lazygit is a terminal UI for git used by tens of thousands of developers daily. One of its power features is the custom patch builder — you cherry-pick individual hunks or files out of commits and move them around. I picked up issue #5802: a hard crash (panic: runtime error: index out of range [-1]) triggered through that feature.

Bug Fix or Performance Improvement

Repro:

Build a custom patch from a commit on branch B.
Check out branch A, which doesn't contain that commit.
Choose "Move patch out into index."
💥 panic: runtime error: index out of range [-1]

Root cause. The patch remembers the hash of the commit it was built from. When the action fires, lazygit looks that hash up in the currently-displayed commits to get an index:

func (self *CustomPatchOptionsMenuAction) getPatchCommitIndex() int {
    for index, commit := range self.c.Model().Commits {
        if commit.Hash() == self.c.Git().Patch.PatchBuilder.To {
            return index
        }
    }
    return -1 // <- not found (e.g. after switching branches)
}

After the checkout, that commit isn't in the list, so the function returns the sentinel -1. That -1 is passed straight into a slice index:

// commits[-1] panics
if commitIndex < len(commits) && commits[commitIndex].IsMerge() { ... }

The existing guard only checks the upper bound. -1 < len(commits) is true, so execution proceeds to commits[-1] and Go panics. And it isn't just the one action — the same -1 flows into all five custom-patch menu handlers (delete-from-commit, move-to-selected-commit, move-into-index, and the two pull-into-new-commit actions).

Code

PR: jesseduffield/lazygit#5865

Rather than sprinkle lower-bound guards on every slice access downstream, I fixed it at the source of the bad value: getPatchCommitIndex() now returns an error when the commit isn't found, and every caller surfaces a friendly message instead of crashing.

func (self *CustomPatchOptionsMenuAction) getPatchCommitIndex() (int, error) {
    for index, commit := range self.c.Model().Commits {
        if commit.Hash() == self.c.Git().Patch.PatchBuilder.To {
            return index, nil
        }
    }
    return -1, errors.New(self.c.Tr.PatchCommitNotInCommitsErr)
}

commitIndex, err := self.getPatchCommitIndex()
if err != nil {
    return err
}

Now, instead of a crash, you get:

Cannot find the commit this custom patch was created from in the current commits list. This can happen after switching branches; recreate the patch to continue.

My Improvements

Fixed the root cause, not the symptom. One unhandled sentinel fed five different crash paths. Guarding each downstream slice access would've been five band-aids; making the lookup return an error fixes the whole class in one place.
Added a regression test that actually reproduces it. lazygit has a great integration-test harness, so I wrote MoveToIndexWhenCommitNotInCurrentBranch: it builds a patch, checks out a branch without the source commit, and moves the patch into the index.
- On master: fails with panic: runtime error: index out of range [-1] — the exact reported crash.
- With the fix: passes (asserts the friendly error popup).
Flagged a sibling. PatchBuildingController has the same return -1 pattern; I noted it in the PR and offered to cover it too, keeping this PR scoped to the reported crash.

Best Use of Google AI

I used Google's Gemini 3.6 Flash (via AI Studio) to independently pressure-test my diagnosis before writing the fix. I gave it the buggy function, the downstream slice access, and the repro, and asked for the root cause and minimal fix. It was sharp:

Root cause: an unhandled sentinel value (-1) representing a missing commit… getPatchCommitIndex() fails to match the hash and returns -1 … commits[-1] triggers Go's runtime panic.

Why the guard fails: commitIndex < len(commits) only checks the upper bound. -1 < len(commits) evaluates to true, so short-circuiting doesn't prevent commits[commitIndex].IsMerge() from accessing commits[-1].

Fix — Option B (idiomatic): Since -1 means the commit was not found, the operation should fail early or display an error rather than rebasing against a non-existent index.

That matched my conclusion exactly — and notably Gemini preferred Option B (guard at the caller and return an error) over Option A (just add a >= 0 bound check), which is the more idiomatic, user-friendly fix and the one I shipped. Where AI got me ~90% of the way (correct root cause + the right fix shape), the remaining human work was the part it couldn't see from a snippet: that the same sentinel hit five handlers, matching lazygit's i18n/error conventions, and proving it with an integration test that fails before and passes after.

Result: a real crash in an 80k⭐ tool, fixed at the root, covered by a test that reproduces the exact panic. PR: #5865 · Issue: #5802.

I made my AI agent break into its own code — and it smashed an IDOR

mohammed faizan mohiuddin — Mon, 27 Jul 2026 09:11:41 +0000

This is a submission for DEV's Summer Bug Smash: Smash Stories powered by Sentry.

The scariest bugs aren't the ones that crash. They're the ones that work perfectly in every demo and quietly hand your data to the wrong person.

Here's one that ships constantly — I've watched AI coding agents write it, confidently, and end the turn with "Done ✅." So I built something to catch it, and this is the first bug it smashed.

The setup

Ask any coding agent for the most boring endpoint in the world:

"Add an endpoint to fetch an invoice by id."

You get this back, and it looks fine:

app.get('/invoice/:id', async (req, res) => {
  const invoice = await db.query(
    `SELECT * FROM invoices WHERE id = ${req.params.id}`);
  res.json(invoice);
});

It passes the demo. GET /invoice/1 returns invoice #1. Ship it.

The smash

There are two bugs hiding in those four lines, and both are invisible on the happy path:

IDOR (broken object-level authorization). Nothing checks that the invoice belongs to the caller. GET /invoice/2 as user 1 cheerfully returns another tenant's invoice. This is the #1 item on the OWASP API Security Top 10 — and it's a one-liner to introduce.
SQL injection. req.params.id is interpolated straight into the query string. GET /invoice/1;DROP TABLE invoices-- is now a conversation you're having with your database.

Neither shows up when you test with your own account. That's exactly why they ship.

What caught it

I'd gotten tired of "Done ✅" on code like this, so I built a skill for my AI agent called the Attacker. The rule is simple: after it writes anything that crosses a trust boundary — untrusted input, auth, a database query — it stops being the author and becomes the attacker. It doesn't recite an OWASP checklist; it actually tries to break this code.

Pointed at the endpoint above, it reported:

Attacked:
- hit /invoice/2 as user 1 → leaked another tenant's row (IDOR) → added owner scope
- non-numeric id → 500 with a stack trace → now a clean 400
- SQL was string-interpolated → parameterized it

Then it fixed the bug at the boundary, not on the one path I happened to test:

app.get('/invoice/:id', async (req, res) => {
  const id = Number(req.params.id);
  if (!Number.isInteger(id)) return res.status(400).json({ error: 'bad id' });

  const invoice = await db.query(
    'SELECT * FROM invoices WHERE id = $1 AND owner_id = $2',
    [id, req.user.id],                 // ownership scope kills the IDOR
  );

  if (!invoice) return res.status(404).end();   // same 404 for "missing" and
  res.json(invoice);                            // "not yours" — don't confirm it exists
});

Three fixes, each aimed at the class of problem:

Parameterized query → the injection is gone, not escaped-and-hoped.
AND owner_id = $2 → the row is scoped to the caller, so the IDOR can't happen even if a new route forgets to check.
Identical 404 for "missing" vs "not yours" → a subtle one: returning 403 for existing-but-forbidden ids leaks which invoices exist. Same response either way closes that enumeration side channel.
400 on a non-numeric id instead of a 500 that dumps a stack trace.

The lesson (worth more than the fix)

The fix is boring. The reflex is the point:

"It works" in a demo is precisely the moment these bugs ship. The only code you trust is the code you already tried to break.

IDOR and injection don't survive because they're hard to fix — they survive because nobody attacks the happy path. Whether it's you, a teammate, or an AI agent writing the endpoint, the discipline is the same: the moment code touches untrusted input or a query, put on the black hat before someone else does.

I ended up bundling the Attacker into an open-source set of "senior-dev instinct" skills for AI agents called Bullpen — but you don't need any of that to steal the habit. Next time your agent says "Done ✅" on an endpoint, ask it one question:

"Now attack it."

I gave my AI coding agent 10 senior-dev instincts

mohammed faizan mohiuddin — Mon, 27 Jul 2026 08:56:42 +0000

AI coding agents are like a junior who read every textbook and shipped nothing:
fast, confident, and wrong in ways that cost you an afternoon.

The senior engineers I've learned the most from aren't valuable because they type
fast. It's their instincts — the things they do before and after the code:

they push back when you ask for the wrong thing
they don't say "done" until they've actually run it
they try to break their own code before it ships
they ask the one question that saves a week of building the wrong feature

So I bottled ten of those into Bullpen — a set of skills for AI coding agents
(built for Claude Code). You call them in by name, at the intensity you want.

The roster

Skill	The instinct
`skeptic`	Pushes back when the request is the wrong idea
`closer`	Won't claim "done" until it ran the code
`attacker`	Red-teams its own code before shipping
`fact-checker`	Verifies an API exists before calling it
`interrogator`	Asks the few decisive questions first
`stop-digging`	After two failed fixes, re-diagnoses instead of thrashing
`doorman`	Justifies every new dependency
`historian`	Checks why odd code exists before deleting it
`chameleon`	Matches the existing codebase's style
`explainer`	Splits work into reviewable commits

Each has lite / full / ultra intensity.

One example: /attacker

Ask an agent for "an endpoint to fetch an invoice by id" and it'll hand you this,
then say "Done":


js
app.get('/invoice/:id', async (req, res) => {
  const invoice = await db.query(
    `SELECT * FROM invoices WHERE id = ${req.params.id}`);
  res.json(invoice);
});

Hit /invoice/2 as user 1 and you read another tenant's invoice. The id
concatenates straight into SQL. With /attacker, it breaks into its own code
first — and fixes it at the boundary:

app.get('/invoice/:id', async (req, res) => {
  const id = Number(req.params.id);
  if (!Number.isInteger(id)) return res.status(400).json({ error: 'bad id' });
  const invoice = await db.query(
    'SELECT * FROM invoices WHERE id = $1 AND owner_id = $2', [id, req.user.id]);
  if (!invoice) return res.status(404).end();
  res.json(invoice);
});

▎ Attacked: /invoice/2 as user 1 → leaked another tenant's row, added owner filter · non-numeric id → 500 with a stack trace, now 400 · SQL parameterized, held.

It never claims "secure." It reports what it tried.

Install

/plugin marketplace add faizanmohiuddin482/bullpen
/plugin install bullpen@bullpen

Open source (MIT), one canonical SKILL.md per skill — no hooks, no bloat.

Repo → https://github.com/faizanmohiuddin482/bullpen

Inspired by Ponytail (https://github.com/DietrichGebert/ponytail) — one lazy
senior dev. Bullpen is the whole room. 🧢

What senior-dev instinct would you add?