feat(diagnostics): assemble the shareable bundle a report is attached to - #5570
aodhanroche wants to merge 1 commit into
Conversation
A report is data; a bundle is the thing somebody actually attaches to a bug report. `DiagnosticsBundle` stages files in a temporary directory, redacts every piece of text on the way in, and hands back zip bytes. It writes nothing into the workspace -- the caller decides where the bytes land -- and it is a context manager so the staging directory goes away even when assembly fails partway. A bundle holds `manifest.json` (what this is, what is in it, and how many values were hidden), a plain-language `README.md` so the recipient does not have to guess what the other files are, `report.json`, `doctor.json`, this session's log, the rotated log files newest-first up to a size budget, and the open workflow when there is a saved file for it. Everything copied in goes through the caller's `Redactor`, not a second one of its own, so one set of redaction counts covers the whole bundle rather than the report alone -- otherwise a manifest could report three values hidden while the log files in the same zip had gone through a different pass entirely. Log files are read as a tail against a byte budget rather than copied whole, so one engine that has been running for a week cannot produce a bundle too large to attach. Anything shortened or left out is recorded as a warning in the manifest, because a truncated log changes what a bundle can prove and a reader has no way to tell otherwise. Paths are canonicalized before being read, so a bundle collected through a symlinked workspace reads the files it means to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| for path in log_files: | ||
| staged_path = f"{LOGS_DIRECTORY_NAME}/{path.name}" | ||
|
|
||
| if any(entry.path == staged_path for entry in self._entries): |
There was a problem hiding this comment.
Note
AI finding, but I checked and I agree with written below.
Nice piece of work overall — one thing worth a look: the duplicate-name guard here (entry.path == staged_path) is a case-sensitive string comparison, but the entry then gets written to the real filesystem via path.write_text in _write. On a case-insensitive filesystem (default macOS/Windows), two source log files named e.g. Engine.log and engine.log would produce different staged_path strings, sail past this check, and then collide on disk — the second write silently overwrites the first's bytes while the manifest still lists both as separate, correctly-sized entries. Might be worth comparing staged paths case-insensitively (or normalizing to lowercase) to keep the guard matching what the filesystem will actually do.
|
|
||
| if remaining <= 0: | ||
| warnings.append( | ||
| f"Log file '{path.name}' was left out because the bundle already holds " |
There was a problem hiding this comment.
Note
AI finding, but I checked and I agree with written below.
Small one: this warning always cites self._max_log_bytes (the full configured budget) rather than how much was actually consumed before hitting remaining <= 0. If an earlier file was skipped for a different reason (e.g. the duplicate-name case above) before the budget was truly exhausted, the message would overstate how much was genuinely used. Might be nice to report self._max_log_bytes - remaining instead, so the manifest gives an accurate picture if someone's chasing down a missing log.
alex-rumsey-foundry
left a comment
alex-rumsey-foundry
left a comment
There was a problem hiding this comment.
Really solid work on this one — the budget/truncation logic is careful and well tested. Two small correctness nits inline: a case-sensitivity gap in the duplicate-log-name guard, and a slightly misleading 'budget exhausted' warning message. Neither is a big deal, just flagging before merge.
2 participants
Issue
A report is data. A bundle is the thing someone actually attaches to a bug report.
Fix
DiagnosticsBundlestages files in a temp dir, redacts text on the way in, and hands back zip bytes — the caller decides where they land. It's a context manager, so the staging dir goes away even if assembly dies partway.Inside:
manifest.json, a plain-languageREADME.md,report.json,doctor.json, this session's log, the rotated logs newest-first, and the open workflow if it has a saved file.Worth a look: it uses the caller's
Redactor, not its own, so one set of counts covers the whole zip — otherwise the manifest could claim three values hidden while the logs beside it went through a different pass. Logs are tailed against a byte budget so a week-old engine can't produce an unattachable zip, and anything shortened is recorded as a manifest warning, since a log starting mid-session looks identical to a session that started there.36 tests.
🤖 Generated with Claude Code