mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(remote): stop the wake handlers shadowing each other; enforce the input cap
Two findings from a final review pass over the wake-on-LAN feature. `_onRemoteHostWaking` / `_onRemoteHostWakeFailed` were defined in BOTH `panels-ui.js` (toasts) and `host-wake-ui.js` (banner). Both files mix into `CodemanApp.prototype` and `host-wake-ui.js` loads later, so the panels-ui copies were silently shadowed: the toast never fired, and a wake started for a BACKGROUND session (input on a non-active tab) produced no notification at all, since the banner handler only acts on the active session. The handlers now live only in `host-wake-ui.js`, show the toast unconditionally, and update the banner when the woken session is the active one. `appendBoundedPending` dropped only WHOLE chunks, so a single input value over the cap (one large paste is one `input` value, up to the 100 KB input schema) was kept in full: "bounded at 4 KB" held per chunk, not per session, and nothing was logged. The surviving chunk's head is now trimmed too, code-point aware so a multi-byte character is never split into a replacement char. Adds the guard that would have caught the first one: every SSE dispatch handler must be defined in exactly ONE frontend module. The existing test only asserts a handler EXISTS somewhere, which two modules both satisfy while one is shadowed.
This commit is contained in:
@@ -404,7 +404,8 @@ The invariants worth keeping:
|
|||||||
after the reattach, with a settle delay so bytes cannot land in a still-connecting
|
after the reattach, with a settle delay so bytes cannot land in a still-connecting
|
||||||
pane. The **send-and-wait** path blocks on the wake instead — its response is open
|
pane. The **send-and-wait** path blocks on the wake instead — its response is open
|
||||||
anyway, and buffering would break the wait contract.
|
anyway, and buffering would break the wait contract.
|
||||||
- **The command runs without a shell** (`spawn(path, [], { shell: false })`), the schema
|
- **The command runs without a shell** (`spawn(path, [], { stdio: 'ignore' })` — `shell`
|
||||||
|
defaults to `false`), the schema
|
||||||
requires a single executable path (no arguments, no `$`/backtick), and `wakeMac` is a
|
requires a single executable path (no arguments, no `$`/backtick), and `wakeMac` is a
|
||||||
structural hex-pair allowlist. A broken or missing wake target fails the wake, never the
|
structural hex-pair allowlist. A broken or missing wake target fails the wake, never the
|
||||||
input route.
|
input route.
|
||||||
@@ -418,8 +419,9 @@ The invariants worth keeping:
|
|||||||
again). Other host-level fields deliberately stay as persisted, so neither path can
|
again). Other host-level fields deliberately stay as persisted, so neither path can
|
||||||
silently re-point an existing pane's SSH options.
|
silently re-point an existing pane's SSH options.
|
||||||
- **UI/SSE**: `remote:hostWaking` and `remote:hostWakeFailed` (plus the reused
|
- **UI/SSE**: `remote:hostWaking` and `remote:hostWakeFailed` (plus the reused
|
||||||
`remote:sessionReconnected`) drive the banner and toasts in `host-wake-ui.js` /
|
`remote:sessionReconnected`) drive the banner and toasts, all from `host-wake-ui.js` —
|
||||||
`panels-ui.js`.
|
its handlers are the ONLY definitions, since a second one in another mixin would be
|
||||||
|
silently shadowed by script order.
|
||||||
|
|
||||||
Tests: `test/remote-wake.test.ts` (decision/throttle table, single-flight registry,
|
Tests: `test/remote-wake.test.ts` (decision/throttle table, single-flight registry,
|
||||||
buffering + flush order, MAC parsing/magic packet, live host-config resolution, and the wiring
|
buffering + flush order, MAC parsing/magic packet, live host-config resolution, and the wiring
|
||||||
|
|||||||
@@ -99,6 +99,11 @@ export function decideRemoteInputAction(args: {
|
|||||||
/**
|
/**
|
||||||
* Append `data` to the pending buffer, dropping the OLDEST bytes when the cap is
|
* Append `data` to the pending buffer, dropping the OLDEST bytes when the cap is
|
||||||
* exceeded. Returns the resulting buffer. Pure.
|
* exceeded. Returns the resulting buffer. Pure.
|
||||||
|
*
|
||||||
|
* A single chunk can itself exceed the cap (one large paste is one `input` value),
|
||||||
|
* so after whole chunks are dropped the surviving chunk's HEAD is trimmed too —
|
||||||
|
* otherwise "bounded at 4 KB" would hold only per chunk, not per session. The trim
|
||||||
|
* is code-point aware, so it never emits a broken multi-byte character.
|
||||||
*/
|
*/
|
||||||
export function appendBoundedPending(
|
export function appendBoundedPending(
|
||||||
pending: string[],
|
pending: string[],
|
||||||
@@ -111,9 +116,25 @@ export function appendBoundedPending(
|
|||||||
total -= Buffer.byteLength(next[0]);
|
total -= Buffer.byteLength(next[0]);
|
||||||
next.shift();
|
next.shift();
|
||||||
}
|
}
|
||||||
|
if (next.length === 1) next[0] = tailWithinBytes(next[0], maxBytes);
|
||||||
return next;
|
return next;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Keep only the trailing part of `value` that fits in `maxBytes` UTF-8 bytes. Pure. */
|
||||||
|
function tailWithinBytes(value: string, maxBytes: number): string {
|
||||||
|
if (Buffer.byteLength(value) <= maxBytes) return value;
|
||||||
|
const chars = [...value];
|
||||||
|
let total = 0;
|
||||||
|
let start = chars.length;
|
||||||
|
while (start > 0) {
|
||||||
|
const size = Buffer.byteLength(chars[start - 1]);
|
||||||
|
if (total + size > maxBytes) break;
|
||||||
|
total += size;
|
||||||
|
start--;
|
||||||
|
}
|
||||||
|
return chars.slice(start).join('');
|
||||||
|
}
|
||||||
|
|
||||||
/** The remote fields the wake flow needs. Structurally satisfied by `SessionRemote`. */
|
/** The remote fields the wake flow needs. Structurally satisfied by `SessionRemote`. */
|
||||||
export interface WakeableRemote {
|
export interface WakeableRemote {
|
||||||
wakeCommand?: string;
|
wakeCommand?: string;
|
||||||
|
|||||||
@@ -314,8 +314,21 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|
||||||
/** SSE `remote:hostWaking` — a wake is running (ours or one started by typing). */
|
/**
|
||||||
|
* SSE `remote:hostWaking` — a wake is running (ours or one started by typing).
|
||||||
|
*
|
||||||
|
* ⚠️ The ONLY definition of this handler: `panels-ui.js` must not define it too.
|
||||||
|
* Both mix into `Codeman.prototype` and this file loads later, so a second copy
|
||||||
|
* would be silently shadowed (the guard in `sse-dispatch-table.test.ts` sees that a
|
||||||
|
* handler exists, not that two modules claim the same name). The toast is
|
||||||
|
* deliberately UNCONDITIONAL — a wake can start for a background session (input on
|
||||||
|
* a non-active tab) where there is no banner to update.
|
||||||
|
*/
|
||||||
_onRemoteHostWaking(data) {
|
_onRemoteHostWaking(data) {
|
||||||
|
const label = data && data.label ? data.label : 'Remote host';
|
||||||
|
// Long enough to cover the wake + attach (~10s measured on a warm S3), and it
|
||||||
|
// is replaced by `remote:sessionReconnected` the moment the pane is back.
|
||||||
|
this.showToast(`Waking ${label} … input is queued`, 'info', { duration: 12000 });
|
||||||
const state = this._hostWake;
|
const state = this._hostWake;
|
||||||
if (!state || !data || state.sessionId !== data.sessionId) return;
|
if (!state || !data || state.sessionId !== data.sessionId) return;
|
||||||
state.waking = true;
|
state.waking = true;
|
||||||
@@ -326,6 +339,8 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
|
|
||||||
/** SSE `remote:hostWakeFailed` — the host did not come back in time. */
|
/** SSE `remote:hostWakeFailed` — the host did not come back in time. */
|
||||||
_onRemoteHostWakeFailed(data) {
|
_onRemoteHostWakeFailed(data) {
|
||||||
|
const label = data && data.label ? data.label : 'Remote host';
|
||||||
|
this.showToast(`${label} did not wake up — queued input is still held`, 'error', { duration: 15000 });
|
||||||
const state = this._hostWake;
|
const state = this._hostWake;
|
||||||
if (!state || !data || state.sessionId !== data.sessionId) return;
|
if (!state || !data || state.sessionId !== data.sessionId) return;
|
||||||
state.waking = false;
|
state.waking = false;
|
||||||
|
|||||||
@@ -120,17 +120,13 @@ Object.assign(CodemanApp.prototype, {
|
|||||||
|
|
||||||
|
|
||||||
// Wake-on-LAN from user input on a sleeping remote host (see remote-wake.ts).
|
// Wake-on-LAN from user input on a sleeping remote host (see remote-wake.ts).
|
||||||
_onRemoteHostWaking(data) {
|
// ⚠️ The `remote:hostWaking` / `remote:hostWakeFailed` HANDLERS live in
|
||||||
const label = data && data.label ? data.label : 'Remote host';
|
// `host-wake-ui.js`, which owns the banner state. They are NOT redefined here:
|
||||||
// Long enough to cover the wake + attach (~10s measured on a warm S3), and it
|
// both files mix into `CodemanApp.prototype` and `host-wake-ui.js` is loaded
|
||||||
// is replaced by `remote:sessionReconnected` the moment the pane is back.
|
// later, so a second definition would silently shadow the banner update (and the
|
||||||
this.showToast(`Waking ${label} … input is queued`, 'info', { duration: 12000 });
|
// toast would never fire — the exact silent no-op `sse-dispatch-table.test.ts`
|
||||||
},
|
// exists to prevent, which cannot see shadowing). The toasts are shown from the
|
||||||
|
// host-wake-ui handlers instead.
|
||||||
_onRemoteHostWakeFailed(data) {
|
|
||||||
const label = data && data.label ? data.label : 'Remote host';
|
|
||||||
this.showToast(`${label} did not wake up — queued input is still held`, 'error', { duration: 15000 });
|
|
||||||
},
|
|
||||||
|
|
||||||
|
|
||||||
// Bash tools
|
// Bash tools
|
||||||
|
|||||||
@@ -76,9 +76,21 @@ describe('appendBoundedPending', () => {
|
|||||||
expect(appendBoundedPending([big], 'newest')).toEqual(['newest']);
|
expect(appendBoundedPending([big], 'newest')).toEqual(['newest']);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('never drops the just-typed chunk even when it alone exceeds the cap', () => {
|
it('trims a single oversized chunk to the cap, keeping its TAIL', () => {
|
||||||
|
// One large paste is one input value, so the cap has to hold WITHIN a chunk too
|
||||||
|
// (otherwise "bounded at 4 KB" would only be true per chunk, not per session).
|
||||||
const huge = 'y'.repeat(REMOTE_WAKE_PENDING_MAX_BYTES + 100);
|
const huge = 'y'.repeat(REMOTE_WAKE_PENDING_MAX_BYTES + 100);
|
||||||
expect(appendBoundedPending([], huge)).toEqual([huge]);
|
const result = appendBoundedPending([], huge);
|
||||||
|
expect(result).toEqual(['y'.repeat(REMOTE_WAKE_PENDING_MAX_BYTES)]);
|
||||||
|
expect(result[0].length).toBe(REMOTE_WAKE_PENDING_MAX_BYTES);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('trims a multi-byte tail without splitting a character', () => {
|
||||||
|
const cap = 10;
|
||||||
|
const value = 'ä'.repeat(8); // 2 bytes each → 16 bytes
|
||||||
|
const result = appendBoundedPending([], value, cap);
|
||||||
|
expect(Buffer.byteLength(result[0])).toBeLessThanOrEqual(cap);
|
||||||
|
expect(result[0]).toBe('ä'.repeat(5)); // 10 bytes, no U+FFFD
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -57,4 +57,22 @@ describe('SSE dispatch table', () => {
|
|||||||
.filter((handler) => !new RegExp(`(^|\\s)${handler}\\s*\\(`, 'm').test(allModules));
|
.filter((handler) => !new RegExp(`(^|\\s)${handler}\\s*\\(`, 'm').test(allModules));
|
||||||
expect(missing).toEqual([]);
|
expect(missing).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('defines every handler in exactly ONE module (a second copy is shadowed)', () => {
|
||||||
|
// Modules mix into `CodemanApp.prototype` and run in script order, so two
|
||||||
|
// definitions of the same handler name silently shadow each other: the later file
|
||||||
|
// wins and the earlier one never runs. The existence check above cannot see that
|
||||||
|
// (both names resolve), which is how a duplicate banner handler can leave a toast
|
||||||
|
// dead with no error anywhere.
|
||||||
|
const byModule = readdirSync(PUBLIC_DIR)
|
||||||
|
.filter((name) => name.endsWith('.js'))
|
||||||
|
.map((name) => ({ name, source: readFileSync(join(PUBLIC_DIR, name), 'utf-8') }));
|
||||||
|
const shadowed = dispatchEntries()
|
||||||
|
.map((entry) => entry.handler)
|
||||||
|
.filter((handler) => {
|
||||||
|
const re = new RegExp(`(^|\\s)${handler}\\s*\\(`, 'm');
|
||||||
|
return byModule.filter((mod) => re.test(mod.source)).length > 1;
|
||||||
|
});
|
||||||
|
expect(shadowed).toEqual([]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user