Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions src/commands/test-as-self.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,35 @@ function resolveCanonicalHome(): string {
return process.env.INSTAR_PROJECT_DIR || process.cwd();
}

/**
* The `instar init` invocation that deploys + initializes the throwaway home at
* an arbitrary `--dir`. Exported + pure so the decision is unit-testable.
*
* MUST NOT be `--standalone`: `init --standalone` requires a positional NAME and
* routes to `~/.instar/agents/<name>` (ignoring `--dir`) — so the prior
* `init --standalone --dir <target>` failed at step 3 unconditionally ("A name is
* required for standalone agents"), which is why this harness never passed.
* `init --dir <target>` (non-standalone → initExistingProject) honors `--dir`,
* allocates a port, and writes `<target>/.instar/config.json` — verified.
*/
export function buildInitArgs(target: string): string[] {
return ['init', '--dir', target];
}

/**
* Env for spawning/stopping the throwaway's OWN server. Strips the PARENT
* session markers so instar's "don't start/stop/restart the server from inside a
* managed session" guard doesn't block the throwaway's lifecycle. The spawn (step
* 4) AND the teardown's `server stop` BOTH need this — without it on teardown,
* `server stop` is refused ("Cannot 'server stop' from inside a session").
*/
export function sanitizedSpawnEnv(base: NodeJS.ProcessEnv): NodeJS.ProcessEnv {
const env = { ...base };
delete env.INSTAR_SESSION_ID;
delete env.INSTAR_JOB_SLUG;
return env;
}

function nowMs(): number { return Date.now(); }

async function sleep(ms: number): Promise<void> { return new Promise((r) => setTimeout(r, ms)); }
Expand Down Expand Up @@ -197,20 +226,19 @@ export async function runTestAsSelf(opts: TestAsSelfOptions): Promise<{ report:
return `throwaway home ${ctx.target}`;
})) return finish(ctx, opts, 2);

// Step 3 — dist deploy (instar init ships the current dist into the home).
// Step 3 — dist deploy (`instar init --dir` initializes the throwaway home).
if (!await runStep('3. dist-deploy', async () => {
execFileSync('node', [ctx.distCli, 'init', '--standalone', '--dir', ctx.target], {
execFileSync('node', [ctx.distCli, ...buildInitArgs(ctx.target)], {
encoding: 'utf-8', timeout: stepDeadlineMs,
env: { ...process.env, INSTAR_NONINTERACTIVE: '1' },
env: { ...sanitizedSpawnEnv(process.env), INSTAR_NONINTERACTIVE: '1' },
});
ctx.port = readPort(ctx.target);
return `deployed; port ${ctx.port}`;
})) return finish(ctx, opts, 3);

// Step 4 — process start (server --no-telegram; lifeline if a bot is set).
if (!await runStep('4. process-start', async () => {
const env = { ...process.env };
delete env.INSTAR_SESSION_ID; delete env.INSTAR_JOB_SLUG;
const env = sanitizedSpawnEnv(process.env);
ctx.serverProc = spawn('node', [ctx.distCli, 'server', 'start', '--foreground', '--no-telegram', '--dir', ctx.target],
{ detached: false, stdio: 'ignore', env });
if (ctx.botToken) {
Expand Down Expand Up @@ -251,7 +279,7 @@ function teardown(ctx: RunContext): void {
try { ctx.serverProc?.kill('SIGTERM'); } catch { /* */ }
// Best-effort: stop any launchd/lifeline the deploy self-installed, then remove the home.
try {
execFileSync('node', [ctx.distCli, 'server', 'stop', '--dir', ctx.target], { encoding: 'utf-8', timeout: 15_000 });
execFileSync('node', [ctx.distCli, 'server', 'stop', '--dir', ctx.target], { encoding: 'utf-8', timeout: 15_000, env: sanitizedSpawnEnv(process.env) });
} catch { /* may not be running */ }
// NOTE: the throwaway home removal is intentionally left to the caller / --keep
// semantics rather than an rm here — SafeFsExecutor is the only sanctioned
Expand Down
33 changes: 33 additions & 0 deletions tests/unit/testAsSelfInit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { describe, it, expect } from 'vitest';
import { buildInitArgs, sanitizedSpawnEnv } from '../../src/commands/test-as-self.js';

describe('buildInitArgs — throwaway home init invocation (the step-3 fix)', () => {
it('uses `init --dir <target>` — honors --dir, creates a runnable home', () => {
expect(buildInitArgs('/tmp/throwaway')).toEqual(['init', '--dir', '/tmp/throwaway']);
});

it('does NOT use --standalone (which requires a name + routes to ~/.instar/agents/<name>, ignoring --dir — the bug that failed step 3 every run)', () => {
expect(buildInitArgs('/tmp/x')).not.toContain('--standalone');
});
});

describe('sanitizedSpawnEnv — strips parent session markers (the teardown fix)', () => {
it('removes INSTAR_SESSION_ID and INSTAR_JOB_SLUG so the in-session server-management guard does not block the throwaway lifecycle', () => {
const out = sanitizedSpawnEnv({ INSTAR_SESSION_ID: 'sess', INSTAR_JOB_SLUG: 'job', PATH: '/usr/bin' });
expect(out.INSTAR_SESSION_ID).toBeUndefined();
expect(out.INSTAR_JOB_SLUG).toBeUndefined();
});

it('preserves every other env var', () => {
const out = sanitizedSpawnEnv({ PATH: '/usr/bin', HOME: '/home/x', FOO: 'bar' });
expect(out.PATH).toBe('/usr/bin');
expect(out.HOME).toBe('/home/x');
expect(out.FOO).toBe('bar');
});

it('does not mutate the input env', () => {
const base = { INSTAR_SESSION_ID: 'sess', KEEP: 'me' };
sanitizedSpawnEnv(base);
expect(base.INSTAR_SESSION_ID).toBe('sess'); // original untouched
});
});
39 changes: 39 additions & 0 deletions upgrades/next/test-as-self-init-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Upgrade Guide — vNEXT

<!-- bump: patch -->

## What Changed

**The throwaway test-deploy harness (`instar test-as-self`) now actually runs.**
It was shipped but had never been executed end-to-end, so a bug went unnoticed:
it always failed at the deploy step because it invoked the wrong form of
`instar init`. It also could not tear itself down cleanly when run from inside a
managed agent session. Both are fixed: the harness initializes the throwaway home
with the directory-targeting form of init, and it strips the parent session
markers when starting and stopping the throwaway's own server.

## What to Tell Your User

Nothing to configure. This is an internal development and testing tool. If you
ever run the throwaway test-deploy harness, it now works end to end and cleans up
after itself, instead of failing immediately.

## Summary of New Capabilities

- `buildInitArgs(target)` and `sanitizedSpawnEnv(env)` (new pure, unit-tested
helpers in `src/commands/test-as-self.ts`): the harness deploys the throwaway
home with `init --dir <target>` (honors the directory, allocates a port) and
runs the throwaway's server lifecycle with the parent session markers removed
so the in-session server-management guard does not block it.

## Evidence

- The harness was run and observed failing at step 3 with "A name is required for
standalone agents", and the teardown was observed hitting "Cannot 'server stop'
from inside a session". The fix was verified: `init --dir` on a fresh directory
creates a runnable home with an allocated port.
- Tests: `tests/unit/testAsSelfInit.test.ts` (5) — the init args use the
directory form and never `--standalone`; the env sanitizer strips the two
session markers, preserves all other vars, and does not mutate its input.
`tsc --noEmit` clean.
- Spec: MULTI-MACHINE-BOOTSTRAP-ROBUSTNESS-SPEC Track F (the harness).
73 changes: 73 additions & 0 deletions upgrades/side-effects/test-as-self-init-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Side effects — fix `instar test-as-self` (init invocation + teardown env)

## What was broken

`instar test-as-self` (the Track-F throwaway-deploy harness) **failed at step 3
on every run** — it was shipped but never actually executed end-to-end, so the
bug was never caught. Two defects, both in `src/commands/test-as-self.ts`:

1. **Step 3 used the wrong init invocation.** It called
`init --standalone --dir <target>`, but `init --standalone` *requires a
positional name* and routes to `~/.instar/agents/<name>` (ignoring `--dir`).
Result: `init` exited with "A name is required for standalone agents" and the
harness reported `VERDICT: FAIL (step 3)` unconditionally.
2. **Teardown's `server stop` was refused.** The teardown ran
`server stop` inheriting the caller's `INSTAR_SESSION_ID`, tripping instar's
"don't start/stop/restart the server from inside a managed session" guard
("Cannot 'server stop' from inside a session"). Step 4's spawn already
stripped those markers; teardown did not.

## The fix

Two pure, exported, unit-tested helpers:
- `buildInitArgs(target)` → `['init', '--dir', target]` — non-standalone init,
which honors `--dir`, allocates a port, and writes `<target>/.instar/config.json`
(verified: `init --dir /tmp/fresh` created a runnable home with a port). Step 3
uses it.
- `sanitizedSpawnEnv(env)` → env minus `INSTAR_SESSION_ID`/`INSTAR_JOB_SLUG`.
Used by the step-4 spawn (replacing the inline delete) AND the teardown's
`server stop`, so the throwaway's own server lifecycle is never blocked by the
parent session's guard.

## Who is affected

- **Anyone running `instar test-as-self`:** it now gets past step 3 and can tear
down cleanly. Before, it was unusable (failed immediately). No other behavior
changes; no flags added/removed.
- **The Track-E two-machine proof:** this harness is its bring-up tool, so this
is a prerequisite unblock.

## Blast radius

- 1 source file: `src/commands/test-as-self.ts` (two helpers + three call sites:
step 3, step 4, teardown). No config/schema/migration. No other command
touched (`init`/`server` unchanged — only how the harness invokes them).

## Failure modes considered

- **Does `init --dir` on a fresh empty dir work?** Yes — verified live: it
allocates a port and writes `.instar/config.json` (no empty-dir rejection; the
only `process.exit` in that path is the prerequisite check, met on a dev box).
- **Side effect of `init --dir`:** it registers the throwaway in the global agent
registry + creates a machine identity. That is the existing `init` behavior,
unchanged by this fix; teardown signals the server but does not unregister the
global entry, so a throwaway leaves a registry row (harmless; under
`~/.instar/test-deploys`).
- **Env sanitization too aggressive?** No — it strips only the two session
markers that trip the guard; every other var (PATH, HOME, …) is preserved, and
the input object is not mutated (both asserted in unit tests).

## Tests

`tests/unit/testAsSelfInit.test.ts` (5): `buildInitArgs` returns `init --dir`
(never `--standalone`); `sanitizedSpawnEnv` strips both markers, preserves other
vars, does not mutate input. `tsc --noEmit` clean. The step-3 premise (`init --dir`
yields a runnable home) was additionally verified by a live run.

## Scope boundary

This change covers the step-3 init invocation and the teardown env. The harness's
step-4 server-start runs the throwaway's server under whatever `node` invokes the
harness, so it is subject to the same native-module (better-sqlite3) node-ABI
matching as any instar server — a property of the runtime, orthogonal to this
change.
Loading