fix(core): fall back to empty on malformed agents.json shape - #29208
fix(core): fall back to empty on malformed agents.json shape#29208soroush5 wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential crash in the agent acknowledgment service caused by corrupted or malformed Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request introduces safe JSON parsing for the acknowledged agents file, falling back to an empty object if the parsed JSON has an unexpected shape, and adds corresponding unit tests. The review feedback correctly identifies that the new isAcknowledgedAgentsMap type guard only performs a shallow check, which could still lead to runtime crashes if nested values are primitives. The reviewer suggests a more robust type guard implementation and additional test cases to cover these nested malformed shapes.
| function isAcknowledgedAgentsMap( | ||
| value: unknown, | ||
| ): value is AcknowledgedAgentsMap { | ||
| return !!value && typeof value === 'object' && !Array.isArray(value); | ||
| } |
There was a problem hiding this comment.
The type guard isAcknowledgedAgentsMap only performs a shallow check on the top-level value. If the JSON is an object but contains non-object values (e.g., {"/project": "string"}), isAcknowledgedAgentsMap will return true. This will cause acknowledge to crash with a TypeError when trying to set a property on a primitive string (e.g., TypeError: Cannot create property 'Agent' on string).
To prevent this, the type guard should also verify that all values within the map are non-null, non-array objects.
function isAcknowledgedAgentsMap(
value: unknown,
): value is AcknowledgedAgentsMap {
if (!value || typeof value !== 'object' || Array.isArray(value)) {
return false;
}
return Object.values(value).every(
(val) => !!val && typeof val === 'object' && !Array.isArray(val),
);
}There was a problem hiding this comment.
You're right — I reproduced it: {"/project": "str"} passes the old guard and then acknowledge() throws TypeError: Cannot create property 'Agent' on string. Fixed by requiring every project entry to be a non-null, non-array object (falls back to empty otherwise), plus regression tests for the nested shapes. Pushed in b8cd71b — 12/12 green.
| ); | ||
| }); | ||
|
|
||
| it.each(['null', '42', '"str"', '[]'])( |
There was a problem hiding this comment.
Add test cases for nested malformed shapes (such as an object containing a string or null value) to ensure the improved type guard prevents crashes on these shapes as well.
| it.each(['null', '42', '"str"', '[]'])( | |
| it.each(['null', '42', '"str"', '[]', '{"/project": "str"}', '{"/project": null}'])( |
There was a problem hiding this comment.
Done — added {"/project": "str"}, 42, null and [] nested cases to the shape test. Without the guard fix, the truthy-primitive ones fail; with it, all 12 pass.
Closes #29207.
A corrupt
agents.json(valid JSON, wrong shape —null, scalar, or array, from an interrupted save, full disk, sync conflict, or hand edit) crashedisAcknowledged/acknowledgewith a rawTypeError, or silently dropped the acknowledgment (array shape).loadnow validates the parsed shape with a type guard and falls back to{}with a logged error, like every other load failure already does.How to test:
nullinto~/.gemini/acknowledgments/agents.jsonand trigger an agent acknowledgment check — no throw, treated as not-acknowledged.npx vitest run src/agents/acknowledgedAgents.test.tsinpackages/core— all 8 pass, including 4 new regression cases (null/42/"str"/[]; the first three fail without the fix).