Fix: keep non-UTF-8 skill resources as Buffers in the skill loaders

[AmaadMartin]

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):
   No existing public issue; found while reading core/src/skills/loader.ts.

2. Or, if no issue exists, describe the change:

Problem: Both decode sites in the skills loader are wrapped in a try/catch that can never fire:

const fileData = await fs.readFile(fullPath);
try {
  files[relativePath] = fileData.toString('utf-8');
} catch (_e: unknown) {
  files[relativePath] = fileData; // unreachable
}

Buffer.prototype.toString('utf-8') never throws in Node — invalid byte sequences are silently replaced with U+FFFD. So the catch was dead code, and a binary skill resource (the canonical case being a PNG under assets/) was stored as a lossy mojibake string whose original bytes are unrecoverable. Consequences, all silent:

• Resources.references/assets is declared Record<string, string | Buffer> (core/src/skills/skill.ts), but neither loader could ever produce the Buffer arm — a published type whose second member was unconstructible by the library's own loaders.
• LoadSkillResourceTool branches on Buffer.isBuffer(content). That branch was unreachable for directory- and zip-loaded skills, so instead of injecting the bytes as an inlineData part it returned the corrupted text as content.
• getSkillResourceFiles (core/src/tools/skill/run_skill_script_tool.ts) does Buffer.from(fileContent).toString(encoding); for a .png the resolved encoding is base64, so the code executor received base64 of the mojibake re-encoding and materialised a corrupt file.
• A binary file under scripts/ passed the typeof src === 'string' filter as mojibake and was registered as a runnable script with corrupted source.

Solution: decode with a fatal TextDecoder and fall back to the raw bytes, via one unexported module-level helper called from both sites:

const UTF8_DECODER = new TextDecoder('utf-8', {fatal: true, ignoreBOM: true});

function decodeUtf8OrBuffer(data: Buffer): string | Buffer {
  try {
    return UTF8_DECODER.decode(data);
  } catch {
    return data;
  }
}

Notes on the choices:

• ignoreBOM: true is load-bearing. The flag name is inverted from how it reads: it means "do not strip a leading U+FEFF". TextDecoder strips the BOM by default while Buffer.toString('utf-8') does not, so without it every BOM-prefixed text resource would silently change content. There is a dedicated test for this, and it fails without the flag (see the mutation evidence below).
• One shared decoder instance is safe — a non-streaming decode() call keeps no state between calls, including after it throws.
• Kept unexported and file-local. Both call sites live in this file; there is no second consumer, so moving it to core/src/utils/ would add a module plus a test file and make the diff longer, not shorter.
• Deliberate, temporary divergence from adk-python. _load_dir and _load_zip_dir in src/google/adk/skills/_utils.py catch UnicodeDecodeError and skip the entry entirely. This PR keeps the bytes instead, because (1) the TS type already promises string | Buffer, and the loadDir doc comment already says "as string for UTF-8 or Buffer otherwise" — this is a docs-vs-code mismatch, not a design change; (2) the consumer is already written and tested for a Buffer; (3) keeping the bytes loses no information, whereas skipping makes the user's PNG vanish with no diagnostic; (4) parity holds where it matters — adk-python's own resource model is dict[str, str | bytes], its GCS loader (_load_files_in_dir) already keeps the bytes on UnicodeDecodeError, and its toolset already handles bytes; only the directory and zip loaders skip. Aligning those two Python loaders is left to a follow-up and no Python file is touched here.
• No feature flag / opt-in. The old behaviour has no legitimate consumer.

Behaviour change under an unchanged public signature (no .d.ts change, no new exports): a caller that previously received a mojibake string for a binary resource now receives a Buffer. LoadSkillResourceTool now takes its binary branch, getSkillResourceFiles now base64-encodes the true bytes, and a binary file under scripts/ is no longer registered as a runnable script (this last one matches adk-python _load_dir and is a strict improvement over executing mojibake).

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus gh pr diff --name-only on every plausibly adjacent PR. Six open PRs also touch core/src/skills/loader.ts (#262, #263, #283, #284, #310, #312), but none of them changes either decode site — grepping their diffs for TextDecoder, toString('utf-8'), decodeUtf8, Buffer.isBuffer, fatal: and ignoreBOM returns nothing in all six. This is overlap in the same file, not a duplicate implementation, and since the six are independent siblings each branched from main there is no single branch to stack on; this PR branches from main and touches only the two decode statements plus additive tests, so it should rebase cleanly against whichever of them lands first.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

10 new it blocks in core/test/skills/loader_test.ts. The hunk is purely additive — no existing test's fixtures or assertions were edited, so the existing loads resources if they exist case still pins the UTF-8 text path.

describe('loadSkillFromDir'): keeps an invalid UTF-8 asset / reference as a Buffer; keeps a truncated multi-byte sequence (e2 82, an incomplete €) as a Buffer; drops an invalid UTF-8 script while a sibling ok.sh survives; decodes multi-byte UTF-8 (héllo — ✓ 🎉) as a string; preserves a leading UTF-8 BOM. describe('loadSkillFromZipBuffer') (new block, archives built with AdmZip): the same asset / reference / script-drop / multi-byte cases. Buffer cases assert both Buffer.isBuffer(...) and byte equality with the original — Buffer.isBuffer alone would also pass against a re-encoded buffer, so the equality is what pins losslessness.

npx vitest run --project unit:core core/test/skills/loader_test.ts
  ✓ 40 tests passed

npx vitest run --project unit:core core/test/skills/loader_test.ts core/test/skills/skill_test.ts core/test/tools/skills/
  ✓ 9 files, 137 tests passed   (consumer suites, unchanged, no regression)

New-code coverage is 100% line and branch: measured with --coverage.include='core/src/skills/loader.ts', the uncovered statement lines are 90,91,92,94,98,99,151,152,224..230,258,305,306,329,330,367,368 and uncovered branch lines 89,97,150,223,228,256,304,328,359,366,379,381,383,385 — none of which is the new decoder (55, 65-70) or either new call site (102, 386). The remaining loader.ts gaps are pre-existing.

Proof the tests can fail (mutation testing). Coverage is a floor, so each new test was run against mutated source and confirmed to fail:

1. Reverted only the loadDir call site to fileData.toString('utf-8') → 4 failures:
   • keeps an invalid UTF-8 asset as a Buffer → AssertionError: expected false to be true // Object.is equality
   • keeps an invalid UTF-8 reference as a Buffer → same
   • keeps a truncated multi-byte sequence as a Buffer → same
   • drops an invalid UTF-8 script → AssertionError: expected { src: '\ufffd\ufffd\ufffd' } to be undefined
2. Reverted only the loadZipDir call site → 3 failures: the two zip Buffer cases with expected false to be true, and drops an invalid UTF-8 script with expected { src: '\ufffd\ufffd\ufffd' } to be undefined.
3. Dropped ignoreBOM: true from the decoder → 1 failure: preserves a leading UTF-8 BOM in text content → AssertionError: expected 'hello' to be '\ufeffhello' // Object.is equality.
4. The two decodes multi-byte UTF-8 content as a string cases (dir + zip) pass both before and after the fix — they are regression guards against an over-eager fallback that would demote all non-ASCII text to Buffer, so they are deliberately not killed by mutations 1-3. They are not unfalsifiable: mutating the helper to always return the raw bytes fails them with expected Buffer[ 104, 195, 169, ... ] to be 'héllo — ✓ 🎉' (that mutation also fails the pre-existing loads resources if they exist case, confirming the text path is guarded).

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Run against the built package (npm run build) with no mocks, using a real 1x1 PNG whose magic bytes 89 50 4e 47 are not valid UTF-8:

1. Create a skill directory: SKILL.md with name: bin-skill, plus assets/logo.png containing the real PNG bytes.
2. const skill = await loadSkillFromDir(dir) → assert Buffer.isBuffer(skill.resources.assets['logo.png']) and that it .equals() the original PNG bytes.
3. Build the same two files into an archive with AdmZip and repeat with loadSkillFromZipBuffer(zip.toBuffer()) → same two assertions.
4. Wire the loaded skill through the real tool: new LoadSkillResourceTool(new SkillToolset([skill])).runAsync({args: {skill_name: 'bin-skill', path: 'assets/logo.png'}, toolContext}).

Result on this branch: both loaders return the exact PNG bytes as a Buffer, and step 4 returns

status: Binary file detected. The content has been injected into the conversation history for you to analyze.

i.e. the tool's binary path is reachable from a real skill directory for the first time. Against the unfixed loader the same script aborts at step 2 with AssertionError: dir: asset must be a Buffer.

Other gates run locally on the pushed commit: npm run build ✓, npm run lint ✓, npm run format:check ✓. npm run ts:check is not part of CI and fails identically with and without this change (308 pre-existing errors, byte-identical set of failing files, none of them core/src/skills/loader.ts or core/test/skills/loader_test.ts).

CI note: the run-tests (windows-latest) leg failed twice on rerun before going green, each time on a different set of integration tests (tests/integration/app_loader/app_loader_test.ts, tests/integration/build_setup/build_setup_test.ts) and always with Test timed out / Hook timed out in a beforeAll that runs npm install in a fixture — never with an assertion failure, and never in a skills test. The same beforeAll times out locally on an unmodified checkout, i.e. it fails before any code from this PR can execute. ubuntu, macOS and the cross-language leg passed on the first attempt; all legs are green now. Raising those fixture-install timeouts is unrelated churn for this PR and is already the subject of separate PRs.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.