fix(code): make plan approval primary option match user's prior mode by jonathanlab · Pull Request #1909 · PostHog/code

jonathanlab · 2026-04-28T15:33:54Z

Problem

Changes

How did you test this?

jonathanlab · 2026-04-28T15:34:10Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

greptile-apps · 2026-04-28T15:48:22Z

Comments Outside Diff (1)

packages/agent/src/adapters/claude/claude-agent.ts, line 1044-1065 (link)

modeBeforePlan not rolled back on failure

modeBeforePlan is written before the await setPermissionMode(...) call, but the catch block only restores permissionMode — leaving modeBeforePlan pointing at a stale value if the RPC fails. On the next successful plan entry the value is overwritten, so this rarely matters in practice, but it could produce a wrong "continue in X mode" label if a failed plan-entry is followed immediately by a successful one from a different prior mode in an unusual error-recovery path.

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/claude-agent.ts
Line: 1044-1065

Comment:
**`modeBeforePlan` not rolled back on failure**

`modeBeforePlan` is written before the `await setPermissionMode(...)` call, but the `catch` block only restores `permissionMode` — leaving `modeBeforePlan` pointing at a stale value if the RPC fails. On the next successful plan entry the value is overwritten, so this rarely matters in practice, but it could produce a wrong "continue in X mode" label if a failed plan-entry is followed immediately by a successful one from a different prior mode in an unusual error-recovery path.

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-options.test.ts
Line: 15-43

Comment:
**Prefer a parameterised test for mode relabeling cases**

The three tests for `default`, `auto`, and `acceptEdits` are structurally identical — call with a `previousMode`, assert `options[0]` has the expected `optionId` and `name`. Per the team's rule of always preferring parameterised tests, these should be collapsed into one table-driven test:

```typescript
it.each([
  ["default", "default", "Yes, continue manually approving edits"],
  ["auto", "auto", 'Yes, continue in "auto" mode'],
  ["acceptEdits", "acceptEdits", "Yes, continue auto-accepting edits"],
])(
  "promotes %s to the first position with the correct continue label",
  (previousMode, expectedId, expectedName) => {
    const options = buildExitPlanModePermissionOptions(previousMode);
    expect(options[0]).toMatchObject({ optionId: expectedId, name: expectedName });
    expect(options[options.length - 1].optionId).toBe("reject_with_feedback");
  },
);
```

This also folds in the "reject option is always last" assertion, making the coverage obvious at a glance.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/claude-agent.ts
Line: 1044-1065

Comment:
**`modeBeforePlan` not rolled back on failure**

`modeBeforePlan` is written before the `await setPermissionMode(...)` call, but the `catch` block only restores `permissionMode` — leaving `modeBeforePlan` pointing at a stale value if the RPC fails. On the next successful plan entry the value is overwritten, so this rarely matters in practice, but it could produce a wrong "continue in X mode" label if a failed plan-entry is followed immediately by a successful one from a different prior mode in an unusual error-recovery path.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(code): make plan approval primary op..." | Re-trigger Greptile}

greptile-apps · 2026-04-28T15:48:26Z

+    expect(options[0]).toMatchObject({
+      optionId: "default",
+      name: "Yes, continue manually approving edits",
+    });
+    expect(options[options.length - 1].optionId).toBe("reject_with_feedback");
+  });
+
+  it("relabels the auto option when it is the previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("auto");
+    expect(options[0]).toMatchObject({
+      optionId: "auto",
+      name: 'Yes, continue in "auto" mode',
+    });
+  });
+
+  it("relabels the acceptEdits option when it is the previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("acceptEdits");
+    expect(options[0]).toMatchObject({
+      optionId: "acceptEdits",
+      name: "Yes, continue auto-accepting edits",
+    });
+  });
+
+  it("ignores an unknown previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("plan");
+    expect(options[0].name).toMatch(/^Yes, /);
+    expect(options[0].name).not.toMatch(/^Yes, continue/);
+    expect(options[options.length - 1].optionId).toBe("reject_with_feedback");
+  });


Prefer a parameterised test for mode relabeling cases

The three tests for default, auto, and acceptEdits are structurally identical — call with a previousMode, assert options[0] has the expected optionId and name. Per the team's rule of always preferring parameterised tests, these should be collapsed into one table-driven test:

it.each([ ["default", "default", "Yes, continue manually approving edits"], ["auto", "auto", 'Yes, continue in "auto" mode'], ["acceptEdits", "acceptEdits", "Yes, continue auto-accepting edits"], ])( "promotes %s to the first position with the correct continue label", (previousMode, expectedId, expectedName) => { const options = buildExitPlanModePermissionOptions(previousMode); expect(options[0]).toMatchObject({ optionId: expectedId, name: expectedName }); expect(options[options.length - 1].optionId).toBe("reject_with_feedback"); }, );

This also folds in the "reject option is always last" assertion, making the coverage obvious at a glance.

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/agent/src/adapters/claude/permissions/permission-options.test.ts Line: 15-43 Comment: **Prefer a parameterised test for mode relabeling cases** The three tests for `default`, `auto`, and `acceptEdits` are structurally identical — call with a `previousMode`, assert `options[0]` has the expected `optionId` and `name`. Per the team's rule of always preferring parameterised tests, these should be collapsed into one table-driven test: ```typescript it.each([ ["default", "default", "Yes, continue manually approving edits"], ["auto", "auto", 'Yes, continue in "auto" mode'], ["acceptEdits", "acceptEdits", "Yes, continue auto-accepting edits"], ])( "promotes %s to the first position with the correct continue label", (previousMode, expectedId, expectedName) => { const options = buildExitPlanModePermissionOptions(previousMode); expect(options[0]).toMatchObject({ optionId: expectedId, name: expectedName }); expect(options[options.length - 1].optionId).toBe("reject_with_feedback"); }, ); ``` This also folds in the "reject option is always last" assertion, making the coverage obvious at a glance. How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

adboio

approving to unblock but i do have a "strong opinion, loosely held" here that i think it's a better UX to have the options always sorted the same way instead of re-sorting every time one is chosen

personally i flip back and forth between the options pretty regularly and this would be mildly frustrating for me

can we check some data to see if that's common, or if users do typically just stick with one option all the time to assess the impact (i.e. would we be getting a net reduction of user inputs)?

adboio · 2026-04-28T18:22:05Z

+    expect(options[0]).toMatchObject({
+      optionId: "default",
+      name: "Yes, continue manually approving edits",
+    });
+    expect(options[options.length - 1].optionId).toBe("reject_with_feedback");
+  });
+
+  it("relabels the auto option when it is the previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("auto");
+    expect(options[0]).toMatchObject({
+      optionId: "auto",
+      name: 'Yes, continue in "auto" mode',
+    });
+  });
+
+  it("relabels the acceptEdits option when it is the previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("acceptEdits");
+    expect(options[0]).toMatchObject({
+      optionId: "acceptEdits",
+      name: "Yes, continue auto-accepting edits",
+    });
+  });
+
+  it("ignores an unknown previous mode", () => {
+    const options = buildExitPlanModePermissionOptions("plan");
+    expect(options[0].name).toMatch(/^Yes, /);
+    expect(options[0].name).not.toMatch(/^Yes, continue/);
+    expect(options[options.length - 1].optionId).toBe("reject_with_feedback");
+  });


jonathanlab · 2026-04-29T11:00:33Z

Merge activity

Apr 29, 11:00 AM UTC: A user started a stack merge that includes this pull request via Graphite.
Apr 29, 11:00 AM UTC: Graphite couldn't merge this pull request because a downstack PR fix(sidebar): show created task immediately #1908 failed to merge.
Apr 29, 11:21 AM UTC: A user started a stack merge that includes this pull request via Graphite.
Apr 29, 11:23 AM UTC: Graphite rebased this pull request as part of a merge.
Apr 29, 11:28 AM UTC: @jonathanlab merged this pull request with Graphite.

When the agent exits plan mode, the dialog now promotes the mode the user was in before plan mode to the top of the option list and labels it "Yes, continue ..." so the natural choice is to resume the prior mode rather than picking from a flat list. Generated-By: PostHog Code Task-Id: 8845a96b-6bc0-42f8-9a9f-c9d572a7b29d

jonathanlab marked this pull request as ready for review April 28, 2026 15:46

greptile-apps Bot reviewed Apr 28, 2026

View reviewed changes

adboio reviewed Apr 28, 2026

View reviewed changes

adboio approved these changes Apr 28, 2026

View reviewed changes

jonathanlab force-pushed the 04-28-fix_code_make_plan_approval_primary_option_match_user_s_prior_mode branch from c67bb56 to 271ba9c Compare April 29, 2026 10:26

jonathanlab force-pushed the 04-28-fix_sidebar_show_created_task_immediately branch from f004c1c to fb0d5fe Compare April 29, 2026 10:26

jonathanlab force-pushed the 04-28-fix_sidebar_show_created_task_immediately branch from fb0d5fe to c904c1b Compare April 29, 2026 11:06

jonathanlab force-pushed the 04-28-fix_code_make_plan_approval_primary_option_match_user_s_prior_mode branch from 271ba9c to 1640b9e Compare April 29, 2026 11:06

jonathanlab changed the base branch from 04-28-fix_sidebar_show_created_task_immediately to graphite-base/1909 April 29, 2026 11:21

jonathanlab changed the base branch from graphite-base/1909 to main April 29, 2026 11:21

jonathanlab force-pushed the 04-28-fix_code_make_plan_approval_primary_option_match_user_s_prior_mode branch from 1640b9e to a0d41ef Compare April 29, 2026 11:22

jonathanlab mentioned this pull request Apr 29, 2026

Make plan approval options contextual to user's current mode #1795

Closed

jonathanlab merged commit f543286 into main Apr 29, 2026
16 checks passed

jonathanlab deleted the 04-28-fix_code_make_plan_approval_primary_option_match_user_s_prior_mode branch April 29, 2026 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(code): make plan approval primary option match user's prior mode#1909

fix(code): make plan approval primary option match user's prior mode#1909
jonathanlab merged 1 commit intomainfrom
04-28-fix_code_make_plan_approval_primary_option_match_user_s_prior_mode

jonathanlab commented Apr 28, 2026

Uh oh!

jonathanlab commented Apr 28, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Apr 28, 2026 •

edited

Loading

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Apr 28, 2026

Uh oh!

adboio Apr 28, 2026

Uh oh!

adboio left a comment

Uh oh!

adboio Apr 28, 2026

Uh oh!

jonathanlab commented Apr 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jonathanlab commented Apr 28, 2026

Problem

Changes

How did you test this?

Uh oh!

jonathanlab commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

adboio Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

adboio left a comment

Choose a reason for hiding this comment

Uh oh!

adboio Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanlab commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonathanlab commented Apr 28, 2026 •

edited

Loading

greptile-apps Bot commented Apr 28, 2026 •

edited

Loading

jonathanlab commented Apr 29, 2026 •

edited

Loading