writeJsonConfigFile in src/main/utils.ts carries nine responsibilities in seventy-five lines of code. Splitting the mechanism from the policy is about forty lines moved inside the same file, adds no dependency and changes no behaviour. Whether it is worth doing is the decision here.
Depends on #1115 landing first, since that is where the function reaches this shape. #1114 is the neighbour and pushes the same way for one of the nine.
What it carries today
| mechanism |
policy |
| an unpredictable temporary name |
refuse when the path is marked unreadable |
| exclusive open, so an entry already at that name is not followed |
serialise the value to JSON |
fsync, tolerating ENOTSUP and EINVAL |
choose 0600 or the umask default by whether the file carries a token |
rename, then fsync of the directory |
resolve the config's symlink, and refuse a cycle |
| remove the temporary only when this call created it |
|
Carrying the existing mode and owner sits between the two: it is mechanism, but which mode and owner is policy, which is why write-file-atomic takes both as arguments rather than deciding them.
Why it is worth naming rather than leaving
Counted by the same marker sweep on both sides, write-file-atomic carries seven responsibilities in 269 lines and writeJsonConfigFile nine in 75. Seven against nine is not the point. All seven of theirs are mechanism, and two are ones this repository does not have at all, cleanup on exit and serialising writes to the same path. They hold no policy: no refusal, no JSON, no choice of mode.
The difference showed up in review rather than in reading. Eight passes over #1115 returned 43 findings, and the last pass returned the largest set of the eight. Its three findings were all interactions between things earlier passes had added, not defects in any one of them:
mkdirSync with the ownership carry: the directory created a moment earlier is root's, so the fallback that looks for an owner reads root and copies it onto the file.- The symlink hop cap with the rename: exhausting the cap returned a link, and the write then renamed a regular file over it.
- The temporary cleanup with the exclusive open: the open refuses a name already taken, and the cleanup deleted it anyway.
While a finding says this does X wrong, more review converges, because there is a finite list and each fix removes one. When it says these two things interact wrong, the pairs grow with the square of the responsibilities, and each fix adds one more that can pair with the rest.
The shape
writeFileAtomic(targetPath, contents, { mode?, owner? }): boolean
writeJsonConfigFile(filePath, data, newFile): boolean
The second reads the mark, resolves the link and refuses when it cannot, decides the mode, finds the owner, serialises, and calls the first. The line falls exactly between the two columns above.
Two of the three interactions above stop being possible: the writer receives a resolved path and an owner rather than deriving them next to the code that creates directories. The third stays inside the writer, which is where it belongs, in twenty lines rather than seventy-five.
What it does not do
Nothing in #1114 is repaired by this. The refusal stays keyed by path, getUserDataDir still creates its directory root-owned under sudo, and a hard-linked config is still detached by the rename. This is about which function holds what, not about what any of them does.
The other option
Leave it. Nine responsibilities in seventy-five lines is not unusual, the function is covered by 645 unit tests including a mutation check per branch, and the interactions above are now fixed and pinned. The cost of the split is a reviewer re-reading a file they have already reviewed eight times.
I have a bias toward doing it, which is why the measurement is here rather than a recommendation. Worked through with Claude Code; the counts on both sides are from the same marker sweep, and the review numbers are from the runs on #1115 rather than recalled.
writeJsonConfigFileinsrc/main/utils.tscarries nine responsibilities in seventy-five lines of code. Splitting the mechanism from the policy is about forty lines moved inside the same file, adds no dependency and changes no behaviour. Whether it is worth doing is the decision here.Depends on #1115 landing first, since that is where the function reaches this shape. #1114 is the neighbour and pushes the same way for one of the nine.
What it carries today
fsync, toleratingENOTSUPandEINVAL0600or the umask default by whether the file carries a tokenrename, thenfsyncof the directoryCarrying the existing mode and owner sits between the two: it is mechanism, but which mode and owner is policy, which is why
write-file-atomictakes both as arguments rather than deciding them.Why it is worth naming rather than leaving
Counted by the same marker sweep on both sides,
write-file-atomiccarries seven responsibilities in 269 lines andwriteJsonConfigFilenine in 75. Seven against nine is not the point. All seven of theirs are mechanism, and two are ones this repository does not have at all, cleanup on exit and serialising writes to the same path. They hold no policy: no refusal, no JSON, no choice of mode.The difference showed up in review rather than in reading. Eight passes over #1115 returned 43 findings, and the last pass returned the largest set of the eight. Its three findings were all interactions between things earlier passes had added, not defects in any one of them:
mkdirSyncwith the ownership carry: the directory created a moment earlier is root's, so the fallback that looks for an owner reads root and copies it onto the file.While a finding says this does X wrong, more review converges, because there is a finite list and each fix removes one. When it says these two things interact wrong, the pairs grow with the square of the responsibilities, and each fix adds one more that can pair with the rest.
The shape
The second reads the mark, resolves the link and refuses when it cannot, decides the mode, finds the owner, serialises, and calls the first. The line falls exactly between the two columns above.
Two of the three interactions above stop being possible: the writer receives a resolved path and an owner rather than deriving them next to the code that creates directories. The third stays inside the writer, which is where it belongs, in twenty lines rather than seventy-five.
What it does not do
Nothing in #1114 is repaired by this. The refusal stays keyed by path,
getUserDataDirstill creates its directory root-owned undersudo, and a hard-linked config is still detached by the rename. This is about which function holds what, not about what any of them does.The other option
Leave it. Nine responsibilities in seventy-five lines is not unusual, the function is covered by 645 unit tests including a mutation check per branch, and the interactions above are now fixed and pinned. The cost of the split is a reviewer re-reading a file they have already reviewed eight times.
I have a bias toward doing it, which is why the measurement is here rather than a recommendation. Worked through with Claude Code; the counts on both sides are from the same marker sweep, and the review numbers are from the runs on #1115 rather than recalled.