[jsweep] Clean write_large_content_to_file.cjs#43312
Conversation
…file.cjs - Sharpen @returns JSDoc from generic {Object} to typed {filename: string, description: string} - Add 5 new test cases: - JSON boolean primitive - JSON string primitive - Array of primitives - Objects with more than 10 keys (truncation check) - Verify returned keys are exactly filename and description - Total test count: 19 (was 14) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work from the jsweep workflow on cleaning up This PR is well-structured and ready for review:
The JSDoc sharpening from
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup of the actions/setup/js/write_large_content_to_file.cjs helper used by gh-aw’s setup scripts, improving type inference for its return value and expanding integration-style test coverage around schema description generation.
Changes:
- Refined the
@returnsJSDoc type forwriteLargeContentToFile()to an explicit{ filename: string, description: string }shape for better TypeScript inference under// @ts-check. - Added 5 new tests to cover JSON primitive handling, array-of-primitives formatting, object key truncation behavior, and exact return keys.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/write_large_content_to_file.cjs | Tightens JSDoc return typing for improved static checking without changing runtime behavior. |
| actions/setup/js/write_large_content_to_file.test.cjs | Adds test cases covering additional generateCompactSchema branches as exercised via writeLargeContentToFile. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, low-risk cleanup. The JSDoc sharpening is correct and the 5 new test cases properly exercise the previously untested code paths in generateCompactSchema. All assertions verified against the implementation — no issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.9 AIC · ⌖ 8.2 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — 2 minor suggestions on assertion robustness; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Regex over-matching (line 209): The truncation test uses
.*which is too permissive — a literaltoBe(...)would pin the actual key list. - Key order coupling (line 218): The return-shape test uses
toEqual(["filename", "description"])which is order-sensitive; prefer a length-check + presence-check pattern.
Positive Highlights
- ✅ Great coverage of all primitive code paths (
boolean,string) that were previously untested - ✅ Array-of-primitives test correctly validates the
[string] (3 items)format - ✅ JSDoc
@returnssharpened from{Object}to{{ filename: string, description: string }}— a real type-narrowing improvement - ✅ Test file structure is consistent with existing patterns (async import, beforeEach/afterEach cleanup)
- ✅ 19/19 tests passing — no regressions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45.2 AIC · ⌖ 5.37 AIC · ⊞ 6.6K
Comment /matt to run again
| const content = JSON.stringify(obj); | ||
| const result = writeLargeContentToFile(content); | ||
|
|
||
| expect(result.description).toMatch(/^\{key0, key1, key2,.*\.\.\.\} \(12 keys\)$/); |
There was a problem hiding this comment.
[/tdd] The .* wildcard lets this assertion pass even if key3–key9 are garbled or missing — tighten to a literal match to actually pin the implementation.
💡 Suggested assertion
expect(result.description).toBe(
"{key0, key1, key2, key3, key4, key5, key6, key7, key8, key9, ...} (12 keys)"
);A literal toBe(...) pins the full truncated key list; the current regex would pass even if middle keys were reordered or omitted.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 4638dca by replacing the permissive truncation regex with an exact toBe(...) assertion in actions/setup/js/write_large_content_to_file.test.cjs.
| const content = JSON.stringify({ a: 1 }); | ||
| const result = writeLargeContentToFile(content); | ||
|
|
||
| expect(Object.keys(result)).toEqual(["filename", "description"]); |
There was a problem hiding this comment.
[/tdd] Object.keys() order is not guaranteed by spec for all JS engines, though V8 preserves insertion order for string keys. Using toEqual(["filename", "description"]) will silently fail if the return object is ever reordered.
💡 More resilient alternatives
Either assert key presence without order concern:
expect(result).toHaveProperty("filename");
expect(result).toHaveProperty("description");
expect(Object.keys(result)).toHaveLength(2);Or use toContain for each expected key:
expect(Object.keys(result)).toContain("filename");
expect(Object.keys(result)).toContain("description");The existing toHaveProperty tests in earlier cases already cover presence; this test is most valuable for asserting no extra keys are returned.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 4638dca by replacing the order-sensitive keys assertion with presence + length checks in actions/setup/js/write_large_content_to_file.test.cjs.
🧪 Test Quality Sentinel Report
📊 Metrics (19 tests)
|
There was a problem hiding this comment.
Non-blocking observations only — not blocking merge
The JSDoc tightening is correct and the five new test cases cover real untested code paths. Two non-blocking issues found:
### Findings (2 medium/low, no blockers)
-
nullprimitive gap (medium) —generateCompactSchemareturns"object"forJSON.parse("null")becausetypeof null === "object"in JavaScript. The new primitive tests cover boolean, string, and array but skip null, leaving this pre-existing bug unexposed. A one-liner guard ingenerate_compact_schema.cjsfixes it; a test here would catch it. -
Brittle key-order assertion (low) —
Object.keys(result).toEqual(["filename", "description"])relies on V8 insertion order, which is reliable in practice but encodes an implementation detail rather than a contract. A length+membership pattern is more robust.
No correctness regressions, no logic errors, no security concerns in the diff.
🔎 Code quality review by PR Code Quality Reviewer · 195.4 AIC · ⌖ 10.7 AIC · ⊞ 5.4K
Comment /review to run again
| expect(result.description).toBe("number"); | ||
| }); | ||
|
|
||
| it("should handle JSON boolean primitive", async () => { |
There was a problem hiding this comment.
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';There was a problem hiding this comment.
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.
|
|
||
| expect(Object.keys(result)).toEqual(["filename", "description"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@copilot merge main and run pr-finisher skill |
|
@copilot please run the Unresolved review feedback to address (newest first):
|
…arge-content-to-file-3795da24ae9284e2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in bb9a027 (with prior feedback fixes in 4638dca): synced with latest |
Addressed by That failed job ran on |
Summary
Cleaned
write_large_content_to_file.cjsas part of the daily JavaScript unbloat workflow.Context Type
Node.js (pure Node module). The file writes content to a temp file and returns a
{ filename, description }metadata object.Changes Made
Type Annotation Improvement
@returnsJSDoc from the vague{Object}to the precise{{ filename: string, description: string }}so TypeScript can infer the return shape without a cast.Test Improvements
Added 5 new test cases to
write_large_content_to_file.test.cjs:booleancode path in generateCompactSchemastringcode path[string] (3 items)format...} (12 keys))Object.keys(result)equals["filename", "description"]Total tests: 19 (was 14, +5 new)
Validation
All checks passed:
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js✓ (19/19 passing)@ts-nocheckwas not present — the file already had@ts-checkenabled.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.