Skip to content

Commit 553b653

Browse files
nawi-25claude
andcommitted
test: address code review — fix timing race, add wildcard ignored-tool test, improve comments
- Remove unreliable 200ms sleep from audit+cloud test: auditLocalAllow is awaited before process.exit so the POST is done by the time the subprocess closes; if it ever races here it would be a production bug too - Add task* wildcard test: task_drop_all_tables must be fast-pathed to allow (documents the intentional security trade-off of user-configured wildcards) - Expand runCheck docstring explaining why cwd=tmpHome is needed alongside HOME=tmpHome (avoids inheriting the repo's own node9.config.json) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 904c3cb commit 553b653

1 file changed

Lines changed: 22 additions & 4 deletions

File tree

src/__tests__/check.integration.test.ts

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ interface RunResult {
3333
* Synchronous runner — safe only when no in-process mock server is involved,
3434
* because spawnSync blocks the event loop (preventing the mock server from
3535
* responding to requests from the child process).
36+
*
37+
* cwd defaults to os.tmpdir() (not the project root) so the subprocess never
38+
* picks up the repo's own node9.config.json and inherits only the HOME config
39+
* written by makeTempHome(). Pass tmpHome explicitly to keep both HOME and cwd
40+
* consistent.
3641
*/
3742
function runCheck(
3843
payload: object,
@@ -43,7 +48,7 @@ function runCheck(
4348
const result = spawnSync(process.execPath, [CLI, 'check', JSON.stringify(payload)], {
4449
encoding: 'utf-8',
4550
timeout: timeoutMs,
46-
cwd, // isolates from project's node9.config.json
51+
cwd, // avoid loading the repo's own node9.config.json
4752
env: {
4853
...process.env,
4954
NODE9_NO_AUTO_DAEMON: '1',
@@ -171,6 +176,19 @@ describe('ignored tools fast-path', () => {
171176
expect(r.status).toBe(0);
172177
expect(r.stdout).toBe('');
173178
});
179+
180+
it('task* wildcard — task_drop_all_tables is fast-pathed to allow', () => {
181+
// "task*" is in ignoredTools; a tool name that looks dangerous but matches
182+
// the pattern must still be silently allowed (the pattern is opt-in by the user)
183+
const r = runCheck(
184+
{ tool_name: 'task_drop_all_tables', tool_input: {} },
185+
{ HOME: tmpHome },
186+
tmpHome
187+
);
188+
expect(r.status).toBe(0);
189+
expect(r.stdout).toBe(''); // no block JSON
190+
expect(r.stderr).not.toContain('blocked');
191+
});
174192
});
175193

176194
// ── 2. Smart rules ────────────────────────────────────────────────────────────
@@ -413,15 +431,16 @@ describe('audit mode + cloud gating', () => {
413431
});
414432

415433
it('audit mode + cloud:true + API key → POSTs to /audit endpoint', async () => {
434+
// auditLocalAllow is awaited in core.ts before process.exit(0), so by the
435+
// time runCheckAsync resolves (process closed) the POST is already complete.
436+
// No sleep needed — if it races here, it's a production bug too.
416437
const r = await runCheckAsync(
417438
{ tool_name: 'bash', tool_input: { command: 'mkfs.ext4 /dev/sda' } },
418439
{ HOME: tmpHome },
419440
tmpHome
420441
);
421442
expect(r.status).toBe(0);
422443
expect(r.stderr).toContain('[audit]');
423-
// Give the server a moment to register the call (auditLocalAllow is awaited)
424-
await new Promise((res) => setTimeout(res, 200));
425444
expect(auditCalls.length).toBeGreaterThan(0);
426445
});
427446

@@ -446,7 +465,6 @@ describe('audit mode + cloud gating', () => {
446465
);
447466
expect(r.status).toBe(0);
448467
expect(r.stderr).toContain('[audit]');
449-
await new Promise((res) => setTimeout(res, 200));
450468
expect(auditCalls.length).toBe(0);
451469
});
452470
});

0 commit comments

Comments
 (0)