fix(app-bundle): fire the plan→executing un-gag reminder for every primary agent - #1153
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesSession reminders
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes the session reminder that keeps agents from behaving as read-only fire for any non-plan agent (develop, research, build, and future ones), not just build, which should fix the reported issue where switching from plan to develop or research left the model stuck in a read-only mindset. The manifest bookkeeping for the new file is internally consistent, and no material defect was found in the new reminder logic that would block merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…imary agent The "you are no longer in read-only mode" switch reminder (build-switch.txt) only fired for the built-in `build` agent (`input.agent.name === "build"`), so a plan→develop or plan→research switch left the model still under the plan-mode reminder — it kept behaving read-only inside an edit-capable director mode. Amicode ships `develop`/`research` as primary agents (autodev→develop, autoresearch→research); upstream's `build`-only allowlist never learned about them. Broaden the condition to the structural complement `!== "plan"` (plan is the only read-only primary), which also matches upstream's OWN experimental-branch guard. No mode-name enumeration, so future primary agents are covered with no further change. Mechanics: forks upstream session/reminders.ts into the overlay — one-line condition change plus a provenance comment, otherwise byte-identical to the pinned base (7fe9938). manifest.json updated surgically (files hash + classification M + counts 807→808 + per_package packages/opencode.M 73→74); exceptions untouched (refresh_manifest.mjs is exceptions-unaware and was NOT used). Drift gate passes: 831 files, overlay↔manifest bijection holds. Verified empirically against real session data: a plan→develop transition with no intervening build turn received no un-gag reminder, while build transitions did.
jeonghun-jj-lee
force-pushed
the
fix/plan-mode-reminder-develop
branch
from
September 14, 2026 18:23
ae929d7 to
22f6c60
Compare
… build:app The in-app "Rebuild Locally" button (dev-tools-rebuild handler) ran `pnpm --filter amicode run build:app` plain, with no --direct-worktree and no AMICODE_DEPLOY_OVERRIDE. build_app_bundle.mjs's #992 deploy guard refuses (exit 1) whenever HEAD ≠ origin/main or the tree is dirty — exactly the state the LOCAL button builds by design (a feature branch / working tree as-is) — so the app-bundle step failed with "app bundle build failed". The shell fallback scripts/rebuild_amicode.sh already does this correctly: local mode passes `build:app -- --direct-worktree` with AMICODE_DEPLOY_OVERRIDE="local rebuild: <branch> working tree", while main mode (freshly synced to a clean origin/main) builds plain. This brings the button to parity with the script. Extracts rebuildAppBundleStep(mode, branch) — a pure helper mirroring the existing rebuildGitCommand pattern — so the local-vs-main divergence is unit-tested (4 new tests). build:binary is untouched: it has no deploy guard, and the script runs it without --direct-worktree too.
…ing out The clipboard-image-read handler spawns up to three sequential osascript calls (file-url → PNG → TIFF), each with a 3 s timeout. With no image on the clipboard all three error out, easily exceeding vitest's 5 s default — so the test was a consistent failure on CI and most dev machines. Mock node:child_process's execFile (the only API the clipboard path uses) to call back with an error immediately. The test still exercises the handler's wiring: message dispatch, visibility gate, reply shape, and the nonce+tab echo — just without depending on a real macOS clipboard or a live osascript process. exec (used by the rebuild handler) is left untouched via the spread.
…estart (#1152) After a rebuild, worktree directories (used by subagent task_spawn sessions) may be deleted while the DB still references them. On server restart the engine bootstraps instances for ALL session directories — hitting ENOENT for stale ones, which cascades into MCP server failures and blocks the chat panel. Instead of deleting sessions (which loses history), repoint each stale session to its parent project's main directory (project.worktree). Both rebuild paths (shell script + in-app Developer Tools button) now run rehome_stale_worktree_sessions / rehomeStaleWorktreeSessions after DB restore and before reload. Also adds bin/ and packs/ to the content-dir deploy lists in both the shell script and atomic_adoption.ts — the MCP server script (bin/dist/mcp-amico.mjs) and pack manifests were never being copied to the installed extension during rebuilds.
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.
Problem
An agent switched into develop (or research) mode keeps behaving as if it's still in plan mode — read-only, refusing to edit.
Root cause is in the engine's per-turn reminder logic (
session/reminders.ts, inherited from upstream). WithexperimentalPlanModeoff (the Amicode default), two reminders are injected off the user message's stored agent:plan.txt— the "READ-ONLY phase" reminder — fires whenagent.name === "plan".build-switch.txt— the "you are no longer in read-only mode" un-gag — fired only whenagent.name === "build".Amicode ships
develop/researchas primary agents (renamed fromautodev/autoresearch), but upstream'sbuild-only allowlist never learned about them. So aplan → developswitch stops the plan reminder (good) yet never delivers the un-gag (bad) — the model stays anchored to the plan-mode framing it accumulated.Fix
Broaden the un-gag condition from an agent-name allowlist to the structural complement:
planis the only read-only primary agent, so!== "plan"is exactly "any executing agent." This:build,develop,research, and any future primary agent — no mode-name enumeration, no coupling to the mode-rename registry;input.agent.name !== "plan") — it's a back-port of their corrected semantics, not an invention.Mechanics
session/reminders.tsinto the overlay: one-line condition change + a provenance comment, otherwise byte-identical to the pinned base (7fe9938).manifest.jsonupdated surgically:fileshash +classification: "M"+counts.overlay_total807→808 +per_package["packages/opencode"].M73→74.exceptionsuntouched. (refresh_manifest.mjswas deliberately not used — it's exceptions-unaware and would duplicate the 23 exception files intofiles, failing the gate.)831 files, overlay↔manifest bijection holds, all hashes verify.Verification
Confirmed empirically against real session data: in a live session, a
plan → developtransition with no interveningbuildturn received no un-gag reminder, while thebuildtransitions in the same session did. Also confirmed the mode switch correctly propagatesagent: "develop"onto user messages (292 user / 2164 assistant develop-tagged messages), so the reminder gap — not a propagation bug — is the cause.Out of scope (pre-existing, flagged)
The drift gate's step 1 fails on pristine
maintoo: the manifest carriesupstream_base_shabut notupstream_base_archive_sha256, so with a materialize cache present it errors. CI passes because it runs the gate without a cache (SKIP path). This PR neither causes nor touches that gap — worth a separate fix.Summary by CodeRabbit