Repository navigation
Conversation
The log and bundle warnings said a write failed without the file or the underlying error. Both now include the relative filename and the reason, such as EACCES: permission denied, and omit absolute paths.
sonukapoor
left a comment
There was a problem hiding this comment.
Approving this, and it is a better piece of work than its size suggests.
The gap is a real one: --debug exists for someone who already has a problem, and when it failed it said only that something could not be written. No file, no reason, so there was no way to tell a read-only directory from a full disk. The success path names the file and the failure path did not, which was the wrong way round.
The hard part is the constraint, and this is what I spent the review on. The previous silence was deliberate, because Node ends its filesystem errors with the absolute path that failed, and that path can carry a username, an internal host, or a token sitting in a directory name. The bundle is the one artefact we tell people is safe to attach to a public issue, so echoing a raw error would reopen exactly the leak that work existed to close.
The sanitiser holds. I made a directory whose name contained both a fake token and an internal hostname, made it unwritable, and ran a real scan into it. Both warnings name the file and the cause, and neither the token, the hostname, the absolute path nor my username appears anywhere in the output. I then put your actual compiled function in front of nine error shapes, including several I expected to get through it: a relative path rather than an absolute one, a Windows drive path, a path with an embedded quote, an error carrying no code at all, and a thrown string rather than an error object. Nothing leaked in any of them.
The part I liked most is what survives. When the error has no path to strip, as with no space left on the device, the actionable detail is kept intact rather than reduced to a bare code. That makes this meaningfully better than printing the code alone, which is all the issue asked for as a minimum.
Everything else lines up. Both messages give the filename relative to the working directory. The scan still completes and the exit code is unchanged: against main on the same unwritable tree, both exit 0 and both report the same 26 findings. Your new test produces a genuine permission failure with a real directory rather than a mocked write, which is what the issue asked for and the thing that makes the absence assertion worth anything. Merge result against current main is clean at 178 suites and 2,465 tests. Bypassing the sanitiser so the raw error is echoed fails 3 of the 10 tests across your two files.
Two observations, neither a change request.
When the strip removes the whole tail of a message that was not a filesystem error, what is left can read oddly, for example "Failed writing to" with nothing after it. It is safe, which is the right priority, and it is only reachable for errors that do not come from the filesystem, so it is cosmetic rather than something to fix now.
When the bundle directory itself could not be created, the name printed is the temporary template including its literal placeholder characters, so someone searching for that exact name will not find it. There is no file to name in that case, so I am not sure there is a better answer, but it is worth knowing it reads that way.
Thanks for taking this one. It is a follow-up with the most awkward constraint of the set attached to it, and you dealt with the constraint rather than working around it.
What changed and why
Follow-up to #1386, as suggested there. After #1330, a failed debug write warns without saying which file or why. Both the debug-log and debug-bundle write-failure warnings now go through
warnWriteFailureand include the relative filename and the underlying reason. The scan still continues, and the exit code is unchanged.Before:
After:
Absolute paths are kept out of the reason text, since they can expose local directory names.
Testing: updated
tests/output/debug.test.ts,tests/output/debug-write-failure.test.tsandtests/e2e/debug-bundle.test.tsto cover both warnings.npm run build, lint and the full suite pass (177 suites, 2447 tests).Closes #1395
Refs #1357