Skip to content

Commit a4263b6

Browse files
committed
Address PR #40 review feedback
- examples/10-toilet-slacking-base.png and examples/11-touch-fish-base.png were JPEGs under a .png extension; the docs ask the agent to pass them as image_synthesize input_file_paths, so the host may sniff by extension and refuse. Re-encode both to real PNGs (same paths, no doc churn). - make_preview_strip.py: when --workdir points at an existing regular file, os.makedirs(..., exist_ok=True) used to leak an uncaught FileExistsError traceback. Reject that case in argument validation with a clear message. - smoke.test.mjs: add two regressions — PNG signature check on the three examples/, and a subprocess invocation that drives the workdir regression through spawnSync.
1 parent 0e1a91d commit a4263b6

4 files changed

Lines changed: 59 additions & 2 deletions

File tree

2.18 MB
Loading
2.1 MB
Loading

‎plugins/weekbin/octopus-meme-maker/scripts/make_preview_strip.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,16 @@ def main():
103103
sys.exit(1)
104104

105105
# A caller-supplied --workdir is never deleted: refusing a non-empty one is
106-
# cheaper than explaining why the user's files are gone.
106+
# cheaper than explaining why the user's files are gone. We also refuse a path
107+
# that already exists as a regular file — `os.makedirs(..., exist_ok=True)`
108+
# would raise an uncaught `FileExistsError` and leak a Python traceback.
107109
cleanup = None
108110
if args.workdir:
109111
workdir = os.path.abspath(args.workdir)
112+
if os.path.exists(workdir) and not os.path.isdir(workdir):
113+
print(f"ERROR: --workdir {workdir} is not a directory. Pass a directory path, or omit "
114+
"--workdir to use a temporary one.", file=sys.stderr)
115+
sys.exit(1)
110116
if os.path.isdir(workdir) and os.listdir(workdir):
111117
print(f"ERROR: --workdir {workdir} is not empty. Pass an empty directory, or omit "
112118
"--workdir to use a temporary one.", file=sys.stderr)

‎tests/plugins/octopus-meme-maker/smoke.test.mjs‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import { test } from 'node:test';
1111
import assert from 'node:assert/strict';
12-
import { readFileSync, existsSync, statSync, readdirSync, writeFileSync, mkdtempSync, mkdirSync, rmSync, unlinkSync } from 'node:fs';
12+
import { readFileSync, existsSync, statSync, readdirSync, writeFileSync, mkdtempSync, mkdirSync, rmSync, unlinkSync, openSync, readSync, closeSync } from 'node:fs';
1313
import { join, resolve } from 'node:path';
1414
import { tmpdir } from 'node:os';
1515
import { spawnSync } from 'node:child_process';
@@ -307,6 +307,26 @@ test('examples/ has 3 base.png samples', () => {
307307
}
308308
});
309309

310+
// The host may sniff the MIME by extension, so the file content must match the
311+
// .png suffix. (Reviewer feedback: these used to be JPEGs under a .png name and
312+
// the docs asked the agent to pass them to image_synthesize.)
313+
test('examples/*.png are actually PNG (signature 89 50 4E 47 0D 0A 1A 0A)', () => {
314+
const PNG_SIG = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]);
315+
for (const f of ['02-stay-late-base.png', '10-toilet-slacking-base.png', '11-touch-fish-base.png']) {
316+
const fd = openSync(join(EX_DIR, f), 'r');
317+
try {
318+
const head = Buffer.alloc(8);
319+
readSync(fd, head, 0, 8, 0);
320+
assert.ok(
321+
head.equals(PNG_SIG),
322+
`${f} is not a real PNG (got bytes ${[...head].map((b) => b.toString(16).padStart(2, '0')).join(' ')})`,
323+
);
324+
} finally {
325+
closeSync(fd);
326+
}
327+
}
328+
});
329+
310330
// ---------------------------------------------------------------------------
311331
// 6b. Host-tool contract vocabulary.
312332
//
@@ -545,6 +565,37 @@ test('make_gif.py refuses output names that cannot work on Windows', () => {
545565
rmSync(base, { recursive: true, force: true });
546566
});
547567

568+
test('make_preview_strip.py rejects --workdir pointing at a regular file (no traceback)', () => {
569+
// Reviewer feedback (PR #40): when --workdir resolves to an existing regular
570+
// file, `os.makedirs(..., exist_ok=True)` used to leak an uncaught
571+
// FileExistsError traceback. The script must fail with a clear argument
572+
// error instead.
573+
if (pythonHasModule('os') !== true) return;
574+
575+
const tmp = mkdtempSync(join(tmpdir(), 'octopus-workdir-'));
576+
const fakeFile = join(tmp, 'not-a-dir.txt');
577+
writeFileSync(fakeFile, 'placeholder');
578+
const video = join(REF_DIR, 'videos', 'breakdown-h3.mp4');
579+
const out = join(tmp, 'preview.png');
580+
581+
const r = spawnSync(
582+
'python3',
583+
[join(SCRIPTS_DIR, 'make_preview_strip.py'), video, out,
584+
'--workdir', fakeFile, '--labels', 'a,b,c,d,e'],
585+
{ encoding: 'utf8' },
586+
);
587+
588+
assert.notEqual(r.status, 0, 'script must exit non-zero on a non-directory --workdir');
589+
assert.match(r.stderr, /--workdir/, 'stderr must name --workdir in the error');
590+
assert.match(r.stderr, /not a directory/, 'stderr must explain the path is not a directory');
591+
assert.doesNotMatch(
592+
r.stderr,
593+
/Traceback|FileExistsError/,
594+
'stderr must not leak a Python traceback',
595+
);
596+
rmSync(tmp, { recursive: true, force: true });
597+
});
598+
548599
test('path containment is compared with the platform normalisation', () => {
549600
// Windows is case-insensitive and accepts both separators, so a raw string
550601
// comparison of dirname(resolved) against the scene dir is wrong there.

0 commit comments

Comments
 (0)