fix(eslint-factory): withhold prefer-structured-clone suggestion when the cloned value carries function properties - #50846
Conversation
…d properties Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…roperties Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
There was a problem hiding this comment.
Pull request overview
Adds a conservative guard preventing unsafe structuredClone suggestions when function-valued properties are detected.
Changes:
- Collects function-property evidence before reporting suggestions.
- Adds regression tests and documentation.
- Includes an unrelated workflow-skill index update.
Show a summary per file
| File | Description |
|---|---|
prefer-structured-clone.ts |
Guards suggestions and updates rule documentation. |
prefer-structured-clone.test.ts |
Tests guarded and safe suggestion paths. |
agentic-workflows/SKILL.md |
Adds an unrelated designer mapping reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| context.report({ | ||
| node, | ||
| messageId: "preferStructuredClone", | ||
| data: { arg: clonedExpressionText }, | ||
| suggest: carriesFunctionProperties |
| - `.github/aw/debug-agentic-workflow.md` | ||
| - `.github/aw/dependabot.md` | ||
| - `.github/aw/deployment-status.md` | ||
| - `.github/aw/designer-mappings.md` |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test block, 4 sub-cases)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: the fix defers reporting to
Program:exitso evidence from the whole file is available — not just a local-scope guard. - Suggestion vs. diagnostic correctly separated: still reports the diagnostic when the guard fires, enabling manual review without silently ignoring the pattern.
- Test coverage is solid: three detection-pattern regression tests plus a safe-path pin.
Minor Suggestions
- Non-identifier boundary test (test file, line 135) — no test pins that member-expression clones (
obj.sub) still receive the suggestion even when the object carries function properties. - Heuristic scope comment (rule file, line 77) — the aliased-variable limitation is not documented inline; future contributors may attempt to extend the guard unnecessarily.
Positive Highlights
- ✅ Excellent
Program:exittwo-pass design — clean and correct. - ✅ All three detection shapes are tested independently.
- ✅
docs.descriptionupdated to explain when the suggestion is withheld. - ✅ PR body references the real affected file with a concrete reproducer.
| ], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The guard at prefer-structured-clone.ts:119 only suppresses suggestions when clonedExpression is an Identifier — non-identifier expressions (e.g. JSON.parse(JSON.stringify(obj.sub))) always get the suggestion. A test pinning this boundary would prevent a future regression if the guard logic is widened.
💡 Suggested test case to add inside this `it` block
{
// member-expression clone: guard does NOT apply even though `tool` carries a function property
code: [
`function register(tool) {`,
` tool.handler = () => {};`,
` return JSON.parse(JSON.stringify(tool.data));`,
`}`,
].join('\n'),
errors: [{
messageId: 'preferStructuredClone',
suggestions: [{ messageId: 'replaceWithStructuredClone', output: ... }],
}],
},@copilot please address this.
| const candidates: { node: TSESTree.CallExpression; clonedExpression: TSESTree.Node }[] = []; | ||
|
|
||
| return { | ||
| AssignmentExpression(node) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The AssignmentExpression visitor only detects direct identifier targets (x.prop = () => {}). Aliased or destructured parameters like const t = tool; t.handler = () => {} bypass the guard. This is an inherent heuristic limitation — consider adding a code comment acknowledging it so future contributors don't try to "fix" the check into something more complex than the risk warrants.
💡 Suggested comment
// Heuristic: only tracks direct identifier targets (x.prop = fn).
// Aliased or destructured variables (const t = tool; t.prop = fn) are not tracked.
// This is intentional — over-suppression is preferable to offering an unsafe suggestion.@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The implementation is correct and well-structured. Defers reporting to Program:exit so file-wide evidence (function-property assignments, object literals with methods, typeof guards) can be collected first. The suggestion is withheld — not the diagnostic — for identifiers carrying function-valued properties. All three evidence patterns handle edge cases correctly, no cross-file leakage from per-create() state, and tests cover all evidence shapes plus the safe path. LGTM.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.8 AIC · ⊞ 5.4K
|
🎉 This pull request is included in a new release. Release: |
prefer-structured-cloneoffered itsstructuredClone(x)suggestion unconditionally, but the two forms are not equivalent:JSON.stringifysilently drops function values whilestructuredClonethrows a synchronousDataCloneError. Applying the suggestion atactions/setup/js/safe_outputs_tools_loader.cjs:244— where the JSON round-trip deliberately stripstool.handlerbefore re-attaching it — would replace working code with code that throws on every call.Rule logic (
src/rules/prefer-structured-clone.ts)Program:exit, so evidence found anywhere in the file is available at report time.suggest: []) when the cloned expression is an Identifier for which the file contains any of:x.p = () => {}/function () {}const x = { p: () => {} },{ p() {} }typeof x.p === "function"checkDocs
meta.docs.descriptionnow states that the suggestion assumes the cloned value carries no function-valued properties, and describes when the guard withholds it.Tests (
src/rules/prefer-structured-clone.test.ts)typeofcheck shape, and the object-literal shape — each expecting an error with an emptysuggestionsarray.generate_safe_outputs_tools.cjs:280that still receives the suggestion, pinning the safe path against regressions.Checked against the real sources with the built rule:
safe_outputs_tools_loader.cjs:244yields 0 suggestions,generate_safe_outputs_tools.cjs:280yields 1.Scope is
eslint-factory/**only; no app code underactions/setup/js/**is touched. The guard is a heuristic — "is this object JSON-safe" is not statically decidable in general — so it errs toward withholding the suggestion rather than offering an unsafe one.