Skip to content

[jsweep] Clean write_large_content_to_file.cjs #43312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 4, 2026
+63 −2
Merged

[jsweep] Clean write_large_content_to_file.cjs #43312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1161cf7
jsweep: strengthen types and expand tests for write_large_content_to_…
github-actions[bot] Jul 4, 2026
500d22c
Merge remote-tracking branch 'origin/main' into signed/jsweep/write-l…
Copilot Jul 4, 2026
4638dca
Address review feedback and merge main
Copilot Jul 4, 2026
bb9a027
Fix null schema expectation test after null handling change
Copilot Jul 4, 2026
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
4 changes: 4 additions & 0 deletions actions/setup/js/generate_compact_schema.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ function generateCompactSchema(content) {
const parsed = JSON.parse(content);

// Generate a compact schema based on the structure
if (parsed === null) {
return "null";
}

if (Array.isArray(parsed)) {
if (parsed.length === 0) {
return "[]";
Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/generate_compact_schema.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe("generateCompactSchema", () => {
expect(generateCompactSchema("123")).toBe("number");
expect(generateCompactSchema('"hello"')).toBe("string");
expect(generateCompactSchema("true")).toBe("boolean");
expect(generateCompactSchema("null")).toBe("object"); // JSON.parse(null) is object
expect(generateCompactSchema("null")).toBe("null");
});

it("should handle invalid JSON", async () => {
Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/write_large_content_to_file.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const { generateCompactSchema } = require("./generate_compact_schema.cjs");
/**
* Writes large content to a file and returns metadata
* @param {string} content - The content to write
* @returns {Object} Object with filename and description
* @returns {{ filename: string, description: string }} Object with filename and description
*/
function writeLargeContentToFile(content) {
const logsDir = "/tmp/gh-aw/safeoutputs";
Expand Down
57 changes: 57 additions & 0 deletions actions/setup/js/write_large_content_to_file.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,61 @@ describe("writeLargeContentToFile", () => {

expect(result.description).toBe("number");
});

it("should handle JSON boolean primitive", async () => {

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing null primitive coverage: the PR adds boolean/string/array primitive tests but omits null, which has a latent bug — typeof null === "object" in generateCompactSchema causes it to hit the object branch but then fall through to return ${typeof parsed}`` returning "object" instead of `"null"`.

💡 Why this matters and suggested fix

JSON.stringify(null) produces the valid JSON string "null". When passed to writeLargeContentToFile, generateCompactSchema parses it, gets JS null, and evaluates typeof parsed === 'object' && parsed !== null — the guard evaluates to false (because null !== null is false), so it falls through to return ${typeof parsed}``, which returns "object" (since `typeof null === 'object'` in JavaScript). A consumer reading the description would incorrectly infer the payload was an object.

Adding this test exposes the bug today:

it('should handle JSON null primitive', async () => {
  const { writeLargeContentToFile } = await import('./write_large_content_to_file.cjs');
  const content = JSON.stringify(null); // → 'null'
  const result = writeLargeContentToFile(content);
  expect(result.description).toBe('null'); // fails: returns 'object'
});

Fix in generate_compact_schema.cjs, before the object branch:

if (parsed === null) return 'null';

Copilot AI Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4638dca: added null handling in actions/setup/js/generate_compact_schema.cjs and added a JSON null primitive test in actions/setup/js/write_large_content_to_file.test.cjs.

const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const content = JSON.stringify(true);
const result = writeLargeContentToFile(content);

expect(result.description).toBe("boolean");
});

it("should handle JSON string primitive", async () => {
const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const content = JSON.stringify("hello world");
const result = writeLargeContentToFile(content);

expect(result.description).toBe("string");
});

it("should handle JSON null primitive", async () => {
const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const content = JSON.stringify(null);
const result = writeLargeContentToFile(content);

expect(result.description).toBe("null");
});

it("should handle array of primitives", async () => {
const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const content = JSON.stringify(["a", "b", "c"]);
const result = writeLargeContentToFile(content);

expect(result.description).toBe("[string] (3 items)");
});

it("should truncate description for objects with more than 10 keys", async () => {
const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const obj = Object.fromEntries(Array.from({ length: 12 }, (_, i) => [`key${i}`, i]));
const content = JSON.stringify(obj);
const result = writeLargeContentToFile(content);

expect(result.description).toBe("{key0, key1, key2, key3, key4, key5, key6, key7, key8, key9, ...} (12 keys)");
});

it("should return both filename and description fields", async () => {
const { writeLargeContentToFile } = await import("./write_large_content_to_file.cjs");

const content = JSON.stringify({ a: 1 });
const result = writeLargeContentToFile(content);

expect(result).toHaveProperty("filename");
expect(result).toHaveProperty("description");
expect(Object.keys(result)).toHaveLength(2);
});
});

@github-actions github-actions Bot Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brittle key-order assertion: expect(Object.keys(result)).toEqual(["filename", "description"]) asserts both presence and insertion order — the test is fragile if the return object is ever restructured or properties are added.

💡 Suggested fix

Use a set-membership check instead:

expect(new Set(Object.keys(result))).toEqual(new Set(['filename', 'description']));
// or
expect(result).toHaveProperty('filename');
expect(result).toHaveProperty('description');
expect(Object.keys(result)).toHaveLength(2);

The intent — verify exactly these two keys and no others — is better expressed through the length + membership pattern. V8 preserves integer-indexed then string-insertion order today, but this is an implementation detail rather than a contract.

Copilot AI Jul 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4638dca by changing this to toHaveProperty checks plus Object.keys(result).toHaveLength(2) in actions/setup/js/write_large_content_to_file.test.cjs.

Loading