mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(release): the seven findings from the pre-release review of the whole tree
A full review of the release tree found seven things, and four of them were mine. **The gate was red, and I put it there.** Splitting `confirmed` into `confirmedContext` and `confirmedSwap` changed the wire field without moving three assertions that check it: `custom-model-one-shot-launch.test.ts` and two in `custom-model-run-menu-ui.test.ts` (the swap modal and the context modal, each of which already receives exactly the right per-question flag). Moved, with the titles. **Worse, my own tests for the split never ran.** The four cases in `session-custom-model.test.ts` that exist specifically to pin it call `mockRunning()`, which was declared inside a sibling `describe`, so they threw a ReferenceError during setup. The split would have shipped with no passing server-side coverage while the gate reported the failure as four broken tests rather than as four tests that were never written. `mockRunning` is hoisted to the outer describe. **The submit verifier pressed Enter into shell panes.** `#455`'s SubmitVerifier resolved its composer glyph as `promptGlyph ?? '❯'`, and only claude and codex declare one, so the other eight modes fell back to claude's `❯`. That is also starship's default shell prompt, and pure's, and spaceship's, and p10k lean's. On such a shell the line `❯ npm run build` sits on screen for as long as the command runs, the verifier reads it as an unsubmitted prompt, and re-presses Enter into the running program's stdin up to nine times on its 2s..60s schedule. Mostly a stray newline; not harmless against a y/N prompt, `read -p`, an installer or a pager, where it takes the default. The module's own fileoverview already stated the rule this broke. Now `?? ''`, which `promptStillInComposer()` already treats as inert, so the verifier runs only for a CLI that actually declares a composer. **My #451 dedent removal left a count behind**: "Two rules keep it honest" introducing three numbered rules. The rest is documentation the split outran. `confirmedContext`/`confirmedSwap` appeared in no doc at all, while `docs/api-reference.md` (the SemVer-covered contract) still told an integrator to retry with `confirmed: true` for both questions, which is precisely the thing the split exists to stop. Documented there, in `docs/custom-model-endpoints.md` and in CLAUDE.md. The custom-model changeset gained the split and the `CLAUDE_CONFIG_DIR` multi-user consequence, both user-visible and both previously absent, and #454's gained the one exception to its own claim: a Custom Endpoints launch ignores the Instance count stepper and always starts one session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -8,4 +8,5 @@ The Instance count stepper next to the Run button only ever applied to Claude.
|
||||
Setting it to 3 and launching OpenCode, Codex, Gemini, Antigravity, Pi, OMP, Grok or
|
||||
DeepSeek started exactly one session, with no error and no hint that the control had
|
||||
done nothing. All eight now launch the count you asked for, and the opening banner
|
||||
says how many are starting.
|
||||
says how many are starting. The one exception is a launch started from the Custom
|
||||
Endpoints section of the Run menu, which always starts a single session.
|
||||
|
||||
@@ -31,5 +31,17 @@ swap window, so a prompt sent mid-swap reads as loading rather than as an answer
|
||||
whatever was loaded a moment ago. A background sweep also catches the reverse: your
|
||||
session's model being evicted later by somebody else's ordinary use.
|
||||
|
||||
Two things worth knowing if you drive this over the HTTP API or run multi-user. The two
|
||||
questions an apply can ask (the model's context window is too small, and loading it will
|
||||
unload the model another session is using) are now answered by separate
|
||||
`confirmedContext` and `confirmedSwap` fields rather than one `confirmed`. They shared a
|
||||
flag until now, and since the context check runs first, confirming that one silently
|
||||
agreed to evict another session's model as well. The old `confirmed` still means both.
|
||||
And `CLAUDE_CONFIG_DIR` is now admin-only in multi-user mode: it joined claude's
|
||||
privileged env keys, so a non-granted owner can no longer set it through `envOverrides`,
|
||||
and an already-persisted one is dropped on reboot-restore, which returns that session to
|
||||
the default Claude account rather than the per-client one it was pointed at. Single-user
|
||||
installs are unaffected.
|
||||
|
||||
Remote SSH and Docker sessions are refused for now, since their restart reattaches a
|
||||
durable tmux rather than relaunching the agent.
|
||||
|
||||
+15
-5
@@ -651,9 +651,17 @@ confirmed? } | { clear: true }` applies (or clears) the session's
|
||||
for a remote (SSH) or Docker session — both restart their agent
|
||||
differently under the hood, and applying to one would report success
|
||||
while changing nothing. Two more responses replace the normal
|
||||
`{customModel, restarted}` shape, neither an error — both require
|
||||
retrying the same call with `confirmed: true` to proceed anyway, and
|
||||
neither restarts or creates anything on the first ask:
|
||||
`{customModel, restarted}` shape, neither an error, and neither restarts
|
||||
or creates anything on the first ask. ⚠️ **Each is answered by its OWN
|
||||
flag on the retry, and answering one is not consent to the other**: they
|
||||
are questions about different people, and while they shared a single flag
|
||||
a caller who confirmed the context warning silently agreed to evict
|
||||
another session's model as well. Send `confirmedContext: true` to proceed
|
||||
past the context warning, `confirmedSwap: true` past the swap conflict,
|
||||
and both when both were asked (they accumulate, so the second retry still
|
||||
carries the first answer). The original `confirmed: true` still means
|
||||
BOTH and is still accepted, because it shipped in this feature's
|
||||
HTTP-API-only cut; new callers should send the specific one:
|
||||
- `{requiresConfirmation: true, currentlyLoadedModel, affectedSessions}` —
|
||||
llama.cpp/llama-swap only runs one model at a time, and switching would
|
||||
unload a model another **live session's own selection** is actively
|
||||
@@ -669,12 +677,14 @@ minSafeContextTokens}` — Claude Code's own fixed per-turn overhead
|
||||
`contextLengthVar` (claude only today), so it never fires for another
|
||||
harness.
|
||||
- `POST /api/v1/quick-start`'s `customModel: { endpointId, modelId,
|
||||
confirmed? }` field (alongside its normal `caseName`/`mode`/etc. body)
|
||||
confirmed?, confirmedContext?, confirmedSwap? }` field (alongside its
|
||||
normal `caseName`/`mode`/etc. body)
|
||||
computes the same injection **before** the session exists and launches
|
||||
directly on the endpoint — no restart, because there was never a
|
||||
native-backend boot to restart away from. Runs the identical checks as
|
||||
the dedicated route above (`requiresConfirmation`/`requiresContextWarning`,
|
||||
same shapes, same `confirmed: true` retry), and is refused the same way
|
||||
same shapes, same per-question `confirmedContext`/`confirmedSwap` retry),
|
||||
and is refused the same way
|
||||
for a remote or Docker case. This is what the Run-menu picker uses for
|
||||
opencode, Codex, Gemini, Pi, Grok, DeepSeek and OMP; Claude still uses the
|
||||
dedicated restart route above (its `--resume`-based restart is far less
|
||||
|
||||
@@ -308,7 +308,7 @@ Invariants:
|
||||
|
||||
Copy goes through `_copyText()` (Clipboard API, then hidden-textarea + `execCommand`), not raw `navigator.clipboard`, because `install.sh`'s LAN option serves plain HTTP where `navigator.clipboard` is undefined; the fallback steals focus, so the terminal is refocused afterwards. Related: xterm registers its own `copy` listener on the terminal element gated on `hasSelection()`, which is why right-click → Copy has always worked. Selection itself is unavailable on touch devices by design (`user-select: none` on the terminal subtree), and in `shell`/`opencode`/`antigravity` tabs the TUI owns the mouse, so selecting there needs Shift+drag. Tests: `test/terminal-copy-selection.test.ts` (gate + wiring invariants), `test/terminal-copy-shortcut.test.ts` (browser, real key presses).
|
||||
|
||||
**The main terminal's four copy paths clean the selection first** (`CodemanCopySelection.clean` in constants.js, pure; `cleanedTerminalSelection()` in terminal-ui.js is the half that reads the live terminal). Those four are the `Ctrl+C` chord, right-click, the phone selection button and Auto Copy. ⚠️ Three routes still copy the RAW padded rows, all of them predating the clean: the browser's own Edit → Copy, which xterm's own `copy` listener on the terminal element serves with `selectionText` directly; a `copy-selection` shortcut the user disabled in App Settings, where nothing calls `preventDefault()` and that native listener runs; and the subagent/teammate windows, which build their own `Terminal` in panels-ui.js with no copy wiring at all. xterm hands back whole screen ROWS and its own trim drops only cells that were never written to, so the real spaces a full-screen TUI paints across the unused part of a row count as content and reach the clipboard. Measured against Claude Code in a 282-column pane, single lines arrived carrying 138 trailing spaces on top of the two-space transcript indent. The clean drops each line's trailing run. Two rules keep it honest:
|
||||
**The main terminal's four copy paths clean the selection first** (`CodemanCopySelection.clean` in constants.js, pure; `cleanedTerminalSelection()` in terminal-ui.js is the half that reads the live terminal). Those four are the `Ctrl+C` chord, right-click, the phone selection button and Auto Copy. ⚠️ Three routes still copy the RAW padded rows, all of them predating the clean: the browser's own Edit → Copy, which xterm's own `copy` listener on the terminal element serves with `selectionText` directly; a `copy-selection` shortcut the user disabled in App Settings, where nothing calls `preventDefault()` and that native listener runs; and the subagent/teammate windows, which build their own `Terminal` in panels-ui.js with no copy wiring at all. xterm hands back whole screen ROWS and its own trim drops only cells that were never written to, so the real spaces a full-screen TUI paints across the unused part of a row count as content and reach the clipboard. Measured against Claude Code in a 282-column pane, single lines arrived carrying 138 trailing spaces on top of the two-space transcript indent. The clean drops each line's trailing run. Three rules keep it honest:
|
||||
|
||||
1. **Trailing padding only. A shared LEADING indent is deliberately NOT stripped**, and that is a decision rather than an omission: it was built, measured and dropped before #451 merged. It looks like the mirror image of the trailing trim and is not, because no native terminal does it and the transform cannot tell a TUI's margin from content that is genuinely indented. Measured over 401,445 three-row windows across 1,010 tracked files in this repo it fired on **73%** of them (92% inside a YAML workflow, 76% over `git log` output, 48% in a TypeScript source), and no width threshold separates the two because they are the same widths: a live Claude Code pane's own margins measure 2 and 5 columns while the most common non-TUI shared run is 4, sitting between them. The failure modes are what settle it. A wrong trailing trim costs nothing; a wrong dedent silently deletes information that was on screen, with no signal and nothing in the clipboard to hint at it, and it is wrong on `git log` bodies, on indented code read out of `cat` (semantic in Python), on `git diff` context rows where the leading space is the marker, and on stack traces. ⚠️ It also could not be made self-consistent cheaply: whether the first row joined the measurement depended on the mousedown COLUMN, which the user never sees, so one block of three rows produced three different clipboard results, and the flag read `getSelectionPosition().start`, which is the mousedown anchor xterm never normalises, so dragging UP through a block read it off the bottom row (the PR's test stub hardcoded a downward drag, so its suite could not express the case). If it is ever revisited, the one qualification that measured clean is **painted trailing padding** (a full-screen TUI writes real spaces across every row, while a shell pane leaves those cells never-written for xterm to trim): zero false positives over all 401,445 windows, no new plumbing. It still mangles a `git log` body sitting inside an agent's own gutter, which is why it was not taken.
|
||||
2. **A COLUMN selection is returned untouched.** Alt+drag makes one (xterm's `shouldColumnSelect` keys on `altKey` alone, and neither `Terminal` Codeman builds passes the one option, `macOptionClickForcesSelection`, that would disable it), and a rectangle's rows lining up is the whole point of the gesture. xterm exposes the mode nowhere public, so the check reads `terminal._core._selectionService._activeSelectionMode` (`SelectionMode.COLUMN` is 3) and cleans normally if a future xterm renames it.
|
||||
|
||||
@@ -227,7 +227,7 @@ already pointed at the endpoint. No restart, because there was never a
|
||||
native-backend launch to restart away from. Runs the same llama-swap
|
||||
conflict check as the restart route (below) — a `409`-shaped
|
||||
`{requiresConfirmation, currentlyLoadedModel, affectedSessions}` response
|
||||
with no session created, resolved by retrying with `confirmed: true` — and
|
||||
with no session created, resolved by retrying with `confirmedSwap: true` — and
|
||||
is refused the same way for a remote or Docker case. This is what the
|
||||
Run-menu picker uses for opencode, Codex, Gemini, Pi, Grok, DeepSeek and OMP;
|
||||
Claude still uses the restart route below (see "The Run-menu picker" above
|
||||
@@ -350,7 +350,8 @@ which can take anywhere from a few seconds to well over a minute:
|
||||
session's own selection** is using it, the apply returns
|
||||
`{requiresConfirmation: true, currentlyLoadedModel, affectedSessions}`
|
||||
instead of silently switching — nothing is applied or created yet.
|
||||
Retrying with `confirmed: true` skips the check. Switching with nothing
|
||||
Retrying with `confirmedSwap: true` skips the check (the legacy `confirmed: true`
|
||||
still means both questions). Switching with nothing
|
||||
else affected proceeds immediately; this is a warning about disrupting
|
||||
another session, never a gate on the switch itself.
|
||||
- **Actually starting the load.** llama-swap has no "switch model" admin
|
||||
@@ -413,7 +414,7 @@ contextLength, minSafeContextTokens}` instead of applying — nothing is
|
||||
restarted or created yet. A context length that was never discovered at
|
||||
all skips the check entirely (nothing to compare, so it fails open rather
|
||||
than warning on every model an endpoint hasn't reported a size for).
|
||||
Retrying with `confirmed: true` launches anyway.
|
||||
Retrying with `confirmedContext: true` launches anyway (the legacy `confirmed: true` still means both questions).
|
||||
|
||||
The Run-menu picker shows this as an in-app modal
|
||||
(`#customModelContextWarningModal`, matching the llama-swap conflict
|
||||
|
||||
+12
-1
@@ -3758,7 +3758,18 @@ export class Session extends EventEmitter {
|
||||
? null
|
||||
: this._mux.capturePaneText?.(this._muxSession.muxName),
|
||||
sendEnter: () => this._mux?.sendInput(this.id, '\r'),
|
||||
glyph: () => getCli(this.mode)?.capabilities.workDetect?.promptGlyph ?? '❯',
|
||||
// ⚠ NO fallback glyph here, unlike the screen-reading probe elsewhere in this file.
|
||||
// Only claude and codex declare a promptGlyph; the other eight modes would fall back
|
||||
// to claude's `❯`, which is ALSO starship's default shell prompt (and pure's, and
|
||||
// spaceship's, and p10k lean's). On a shell session the line `❯ npm run build` sits
|
||||
// on screen for as long as the command runs, promptStillInComposer() reads that as
|
||||
// "still unsubmitted", and the verifier presses Enter into the running program's
|
||||
// stdin on its 2s..60s schedule. Mostly a stray blank line; not harmless against a
|
||||
// y/N prompt, `read -p`, an installer or a pager, where it takes the default.
|
||||
// promptStillInComposer() returns undefined for an empty glyph, so this makes the
|
||||
// verifier inert for every CLI that does not declare one, which is what the Claude
|
||||
// Code 2.1.277 defect it exists for actually calls for.
|
||||
glyph: () => getCli(this.mode)?.capabilities.workDetect?.promptGlyph ?? '',
|
||||
log: (m) => console.log(`[Session ${this.id.slice(0, 8)}] ${m}`),
|
||||
});
|
||||
this._submitVerifier.arm(text);
|
||||
|
||||
@@ -154,7 +154,7 @@ describe('_quickStartWithCustomModelConfirm', () => {
|
||||
expect(app._lastCustomModelLaunchResult).toEqual(data.data);
|
||||
});
|
||||
|
||||
it('confirming re-sends with confirmed:true and returns the second response', async () => {
|
||||
it('confirming re-sends with confirmedSwap and returns the second response', async () => {
|
||||
const { win, app } = bootApp();
|
||||
app._confirmModelSwap = async () => true;
|
||||
let calls = 0;
|
||||
@@ -170,7 +170,10 @@ describe('_quickStartWithCustomModelConfirm', () => {
|
||||
},
|
||||
};
|
||||
}
|
||||
expect(body.customModel.confirmed).toBe(true);
|
||||
// the SWAP question's own flag, never the blanket `confirmed`: answering this one
|
||||
// must not also silence the context-floor warning.
|
||||
expect(body.customModel.confirmedSwap).toBe(true);
|
||||
expect(body.customModel.confirmed).toBeUndefined();
|
||||
return { success: true, data: { sessionId: 's1', modelSwapInProgress: true } };
|
||||
});
|
||||
const data = await app._quickStartWithCustomModelConfirm({
|
||||
|
||||
@@ -574,7 +574,7 @@ describe('Custom Model Endpoint Profiles: llama-swap model-swap confirmation and
|
||||
expect(confirmMessage).toContain('qwen3');
|
||||
expect(applyBodies).toEqual([
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3' },
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3', confirmed: true },
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3', confirmedSwap: true },
|
||||
]);
|
||||
});
|
||||
|
||||
@@ -1041,7 +1041,7 @@ describe("Custom Model Endpoint Profiles: requiresContextWarning (this CLI's own
|
||||
expect(confirmArgs).toEqual(['qwen3', 16384, 40000]);
|
||||
expect(applyBodies).toEqual([
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3' },
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3', confirmed: true },
|
||||
{ endpointId: 'llama-box', modelId: 'qwen3', confirmedContext: true },
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
@@ -39,6 +39,17 @@ async function setup(ctxOptions?: Parameters<typeof createRouteTestHarness>[1])
|
||||
}
|
||||
|
||||
describe('POST /api/sessions/:id/custom-model', () => {
|
||||
/** Shared by the conflict-check block and the context-floor block below, which needs
|
||||
* both conditions true at once. Scoped to the outer describe on purpose: while it
|
||||
* lived inside the conflict-check block, a sibling calling it threw a ReferenceError
|
||||
* during setup, so those tests reported as failing rather than as not written. */
|
||||
function mockRunning(running: Array<{ model: string; state: string }>) {
|
||||
fetchMock.mockImplementation(async (url: URL) => {
|
||||
if (url.pathname === '/running') return new Response(JSON.stringify({ running }), { status: 200 });
|
||||
throw new Error(`unexpected request in this test: ${url.href}`);
|
||||
});
|
||||
}
|
||||
|
||||
beforeEach(async () => {
|
||||
await writeCustomModelHosts(getDataDir(), []);
|
||||
fetchMock.mockReset();
|
||||
@@ -229,13 +240,6 @@ describe('POST /api/sessions/:id/custom-model', () => {
|
||||
});
|
||||
|
||||
describe('llama-swap conflict check (llama.cpp runs one model at a time)', () => {
|
||||
function mockRunning(running: Array<{ model: string; state: string }>) {
|
||||
fetchMock.mockImplementation(async (url: URL) => {
|
||||
if (url.pathname === '/running') return new Response(JSON.stringify({ running }), { status: 200 });
|
||||
throw new Error(`unexpected request in this test: ${url.href}`);
|
||||
});
|
||||
}
|
||||
|
||||
it('applies straight away when the requested model is already loaded', async () => {
|
||||
const { app, ctx } = await setup();
|
||||
ctx.sessions.get('test-session-1')!.mode = 'claude';
|
||||
|
||||
Reference in New Issue
Block a user