Merge pull request #89 from deepseek-ai/worktree-lessons-learned

docs(agents): stacked-PR orchestration + sharper real-entry-path lessons

AGENTS.md

@@ -24,6 +24,16 @@ When carrying out the change fights back — a removal forces an awkward migrati
 
 The worked example is [Keep one public stop primitive](docs/rfc/implemented/simplification/2026-06-20-public-agent-stop-surface.md): it proposed removing BOTH `Agent.abort()` and `Agent.whenIdle()` as redundant stop/quiescence surface. Validating against the code, `abort()` was genuinely dead — no production caller, the loop aborts its own `AbortController` directly — so it was removed as proposed. But `whenIdle()` was load-bearing: a deliberate quiescence primitive with live ACP consumers, and the RFC's suggested migration (observe the `running`→`idle` transition by hand) is exactly the brittle path § Defensive patterns warns against ("Async state is not synchronous state"). So only `abort()` shipped, `whenIdle()` stayed, and the RFC's text was amended on the way to `implemented/` to record the narrowed scope — the landed RFC is not a lie about what was built.
 
+## Orchestrating review feedback across a stacked PR chain
+
+A wave of review comments lands across several PRs in a dependent stack (`A ← B ← C …`) at once. Resolving it well is a discipline of its own, learned the hard way:
+
+- **One worktree per PR branch; never rewrite a pushed branch.** Each PR's fixes happen in that PR's own worktree. To bring a child up to date with a parent's new commits, **merge the parent down** — never rebase/amend/force-push a branch that is already pushed (see [§ Conventions](#conventions) "Never rewrite a pushed branch"). The stacked-merge graph and the per-round review-fix history depend on it.
+- **A fix belongs on the PR that INTRODUCED the issue, then flows DOWN.** When a comment on PR `B` points at code `B` introduced, fix it on `B` and merge `B` into `C` — even if `C` already carries the same file through the chain. Originating the fix on the downstream `C` leaves `B` shipping the unfixed code and the fix invisible to a reviewer of `B`. (This bit us: a snapshot-test guard flagged on the lower PR got fixed only on the top PR, so the lower PR still read as unaddressed until the fix was relocated to its true origin and merged down.)
+- **Each review fix is a SEPARATE commit, never an amend.** The "fix review findings" commit is part of the record — it shows what the review caught and how. Amending erases that. (Amend is fine only for your own not-yet-pushed work.)
+- **Delegated work is trust-but-verify.** When sub-agents implement fixes in parallel, their report describes what they INTENDED, not necessarily what landed. Re-run the gates yourself on the actual tree, and for a regression guard, **prove it FAILS on the unfixed code** (introduce the regression, watch the test go red, revert) — a guard that passes both ways guards nothing. A sub-agent that "reframes the problem as already-handled" instead of fixing it is a signal to dig in personally, not to accept the reframing.
+- **Triage on the merits, then reply in-thread.** Verify each comment against the code before acting (a reviewer flagging the right symptom can still mis-diagnose the cause — confirm both). Reply in the GitHub review thread (`gh api …/pulls/{pr}/comments/{id}/replies`), not as a top-level comment, stating the fix and the commit that carries it.
+
 ## Architecture
 
 This codebase is based on the **Cordis** framework, built microkernel-style: **everything is a plugin**. All necessary Cordis dependencies are copied into this monorepo as vendored source (under `vendor/`) instead of being depended on via npm.
@@ -192,6 +202,7 @@ Dev/test/demo run **unbuilt** via tsx + the `paths` map in the root `tsconfig.js
 - **An empty `catch` must name what it swallows and why nothing else can hit it**: a bare `catch {}` hides bugs. When you deliberately ignore a throw, the comment must (a) name the single expected failure, (b) say why ignoring it is correct — usually because the useful state was already captured *before* the `try` — and (c) make clear nothing else of consequence can reach the catch (ideally the `try` wraps a single statement). Example: the error-body `response.json()` parse in `dsh-llm-deepseek`'s adapter sets `code` + HTTP `status` from the status line before the `try`, so a malformed provider body can only cost a richer message, never the real error.
 - **Symmetry is usually more correct**: when two related values play parallel roles (a test fixture and its expected output, a request shape and its response shape, a buggy input and the test that checks the fix), give them parallel form — both named consts, or both inline, not one each way. Asymmetry is a smell that usually points at a missed extraction.
 - **Merging PRs**: always merge with a **merge commit** (`gh pr merge --merge`), never squash or rebase. The per-PR commit history is intentional — review-fix commits, regression-test commits, and the reasoning in each message are part of the record — and squashing flattens it away.
+- **Never rewrite a pushed branch in a stacked chain.** Once a branch is pushed (and especially once it has a PR), do NOT `rebase`, `amend`, or force-push it. Update a child branch by **merging its parent down** (`git merge <parent-branch>` into the child, as a new merge commit), never by rebasing the child onto the parent's new tip. Rewriting a shared branch diverges it from what the parent and GitHub recorded, which breaks the stacked-merge graph and erases the review-fix history that documents what each round caught. Amending is fine ONLY for your own not-yet-pushed, not-yet-reviewed work. A corollary on WHERE a fix lands: a review fix belongs on the PR that **introduced** the issue, even when a downstream PR in the stack also carries the affected file — fix it on the originating branch, then merge that branch DOWN the chain, rather than originating the fix on the downstream PR (where it would be invisible to a reviewer of the PR that actually owns the code).
 - **TODO markers**: use `FIXME`/`TODO`/`XXX` to flag known issues by urgency — see [docs/development.md](docs/development.md) for the semantics of each.
 - **Tests**: vitest, colocated under `packages/<group>/<pkg>/tests/*.spec.ts`. Every registry needs an HMR-safety test (dispose the contributing fiber, assert cleanup). **Excessive tests are welcome** — when in doubt, write the test; err on the side of covering edge cases, error paths, event ordering, and concurrency races even if they seem unlikely. Review findings get regression tests (see `packages/core/agent-loop/tests/review-fixes.spec.ts`). The same generosity applies to **real-API (with-key) e2e tests — inference is cheap here (we are DeepSeek), so do not ration them**: cover the agent's real flows (a real prompt that writes a file, multi-turn, tool use, cancellation) and run them frequently while developing, especially cheap **smoke tests** that boot the real example and check the world. A green mock/no-key suite proves the plumbing, not the product — the with-key smoke test is what catches "green units, broken product". See § Secrets / .env for the with-key policy and why self-skip is a CI accommodation, not a verdict that real-API tests are expensive.
 - **Prefer the REAL implementation over a mock/stand-in in tests.** When the genuine collaborator is available in the repo, wire it up instead of hand-rolling a fake — a test that registers an inline `defineTool({ name: 'bash', … })` to stand in for `dsh-tool-bash` proves the *bridge* moves bytes but not that the *shipping tool* renders the way the test asserts; the two drift and the test passes while the product is wrong. Mock only the genuinely expensive/non-deterministic boundary (the LLM adapter, the network, the clock) and keep everything downstream real: a bridge tool-call test runs the scripted mock MODEL but the REAL tool + REAL executor (e.g. `makeBridgeHarness({ withBash: true })` plugs `dsh-bash-local` + `dsh-tool-bash` and runs an actual `echo`), so it verifies the actual `presentCall`/`presentResult` an editor sees. This is the unit-test echo of "verify the world, not a synthetic stand-in" (see § Defensive patterns) — a fake you wrote will agree with whatever you assumed; the real thing won't.
@@ -208,7 +219,9 @@ Each bullet is a bug class that bit us; the rule prevents the reoccurrence.
 - **Contain callback exceptions at the boundary.** A user-supplied listener (`onTaskDone`, event handlers) that throws must not reject the promise it runs inside or starve the listeners after it. Wrap the dispatch loop in try/catch and log; never let one bad subscriber break core lifecycle.
 - **Never hand untrusted/model output the ambient environment or predictable paths.** Spawned commands get a scrubbed env (drop `*KEY*`/`*SECRET*`/ `*TOKEN*`) so the harness's own credentials can't leak into output, `env`, or spill files. Temp/spill files use a private (0700) dir, random names, and exclusive owner-only (`'wx'`, `0o600`) opens — predictable world-readable paths invite symlink races and disclosure.
 - **e2e tests own their resources.** Real-API/integration tests must create the harness in the test and dispose it in `afterEach` (even on failure/retry/timeout), so a flaky run doesn't leak processes or contexts. Shared fixtures live in a plain `tests/harness.ts` module, NOT another `*.e2e.ts` file — importing a spec file re-registers its `describe` and duplicates real API calls. Verify the WORLD, not the agent's self-report: re-run the command/check externally and assert files are byte-identical where they should be unchanged (a keyword probe lets a cheating agent pass).
-- **Line coverage is not behavior coverage; test the REAL entry path, not a synthetic stand-in.** 100% per-file coverage and a green suite are necessary, not sufficient — they prove lines ran, not that the feature works the way it ships. A plugin shipped via `cordis.yml` is loaded by the cordis Loader, which calls `Loader.unwrapExports` (`exports.default ?? exports`) and then constructs a fiber from the module's `inject`/`name`/`Config` namespace exports. A test that mounts the plugin by hand-building `ctx.plugin({ name, inject, apply })` (or even `ctx.plugin(NamespaceImport)`) BYPASSES `unwrapExports` entirely, so it cannot catch a broken export shape. This bit us hard: a stray `export default apply` made `unwrapExports` collapse the module to the bare function, dropping `inject` — so every service read threw `cannot get property … without inject` the instant a real editor connected, while 178 hand-mounted tests stayed green. The guard is at least one test that drives the plugin through its REAL load path (a subprocess booting the example via the Loader, or the Loader API directly), exercising the headline operations end-to-end. It runs WITHOUT a key when the operation doesn't call the model (`session/new`/`session/load` reach the factory but never the LLM), so there is no excuse to skip it. Corollary: when an `*.e2e.ts` spawns the example from a temp cwd, set `TSX_TSCONFIG_PATH` to the repo-root tsconfig — the unbuilt `paths` map is found by searching UP from cwd, so a temp cwd outside the repo silently falls back to built `lib/`, which both hides source changes and only "works" when a stale build happens to exist.
+- **Line coverage is not behavior coverage; test the REAL entry path, not a synthetic stand-in.** 100% per-file coverage and a green suite are necessary, not sufficient — they prove lines ran, not that the feature works the way it ships. A plugin shipped via `cordis.yml` is loaded by the cordis Loader, which calls `Loader.unwrapExports` (`exports.default ?? exports`) and then constructs a fiber from the module's `inject`/`name`/`Config` namespace exports. A test that mounts the plugin by hand-building `ctx.plugin({ name, inject, apply })` (or even `ctx.plugin(NamespaceImport)`) BYPASSES `unwrapExports` entirely, so it cannot catch a broken export shape. This bit us hard: a stray `export default apply` made `unwrapExports` collapse the module to the bare function, dropping `inject` — so every service read threw `cannot get property … without inject` the instant a real editor connected, while 178 hand-mounted tests stayed green. The guard is at least one test that drives the plugin through its REAL load path (a subprocess booting the example via the Loader, or the Loader API directly), exercising the headline operations end-to-end. It runs WITHOUT a key when the operation doesn't call the model (`session/new`/`session/load` reach the factory but never the LLM), so there is no excuse to skip it. Corollary: when an `*.e2e.ts` spawns the example from a temp cwd, set `TSX_TSCONFIG_PATH` to the repo-root tsconfig — the unbuilt `paths` map is found by searching UP from cwd, so a temp cwd outside the repo silently falls back to built `lib/`, which both hides source changes and only "works" when a stale build happens to exist. Two sharper corollaries this bit us with again:
+  - **A real-load-path test only GUARDS the export shape if a broken shape actually FAILS it.** The original crash (`cannot get property … without inject`) fired because that plugin HAS `inject`. A plugin with NO `inject` (a composition/bundle plugin that mounts children carrying their own inject, e.g. `dsh-agent-core` and the app packages) does NOT crash on a stray `export default` — `unwrapExports` silently drops `Config`/`name` and the plugin boots anyway — so a Loader smoke stays green while the export shape is broken. For such plugins add an EXPLICIT assertion that the regression fails: `expect('default' in mod).toBe(false)` plus running the module through the real `Loader.prototype.unwrapExports` and asserting `name`/`Config`/`apply` survive. Prove it: add `export default apply`, watch the test go red, revert.
+  - **"Real entry path" means the PUBLISHED ARTIFACT, not the dev runtime.** A test (or a `demo:*` smoke) that boots `src/bin.ts` under `tsx` is NOT the same code a consumer runs — the package `bin` field points at the built `lib/bin.js` under plain `node`. tsx masks failure modes the published artifact has: a boot settle-race that exits 0 before the app's handles attach, module-resolution differences (the unbuilt `paths` map vs node_modules), and a load failure that `loader.await()`'s `Promise.allSettled` SWALLOWS so a typo'd config silently exits 0. The guard is a smoke that runs the built `lib/bin.js` under plain `node` in a node_modules-shaped temp dir (symlinked workspace + vendor packages), asserts the real output, AND asserts a genuinely-missing config exits NON-ZERO. The tsx demo is necessary but not sufficient; the published-bin smoke is what catches "green under tsx, broken on install".
 - **Tag spelling and EOF hygiene.** cordis.yml interpolates env via the `!!js` tag (js-yaml resolves custom tags under `tag:yaml.org,2002:js`), not `!js` — keep code, comments, and docs consistent. Files end with exactly one trailing newline; `git diff --check` (a pre-push gate) rejects new blank lines at EOF.
 
 ## Type Safety and Documentation