Skip to content

Skip user-authored lines in attachment size injection#323

Merged
jeremy merged 2 commits intomainfrom
fix-chat-attachment-display
Mar 16, 2026
Merged

Skip user-authored lines in attachment size injection#323
jeremy merged 2 commits intomainfrom
fix-chat-attachment-display

Conversation

@jeremy
Copy link
Copy Markdown
Member

@jeremy jeremy commented Mar 16, 2026

Summary

  • injectAttachmentSizes now requires marker lines to be preceded by an empty line before annotating with file size
  • Matches the deterministic \n📎 filename\n output from HTMLToMarkdown, preventing false matches on user-authored content

Addresses post-merge review feedback on #322.

Test plan

  • New test: user-authored 📎 report.pdf after prose is left alone, real marker after blank line gets annotated
  • Updated duplicate filenames test to use blank-line-separated markers
  • All existing tests pass
  • Lint, vet, unit tests, e2e all green

Summary by cubic

Update injectAttachmentSizes to annotate only markers that match provided attachments, including inline markers after content, while leaving user-authored "📎 ..." lines untouched.

  • Bug Fixes
    • Restores annotation for inline markers from HTMLToMarkdown ("content\n📎 filename\n").
    • Keeps non-matching "📎 ..." lines unchanged; adds a test for inline attachments.

Written for commit 3cebe10. Summary will update on new commits.

injectAttachmentSizes now only annotates marker lines preceded by an
empty line, matching the deterministic "\n📎 filename\n" output from
HTMLToMarkdown. User-authored standalone 📎 lines following prose
content are left untouched.
@jeremy jeremy requested a review from a team as a code owner March 16, 2026 04:20
Copilot AI review requested due to automatic review settings March 16, 2026 04:20
@github-actions github-actions bot added commands CLI command implementations tests Tests (unit and e2e) labels Mar 16, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens injectAttachmentSizes so it only annotates attachment marker lines when they match the deterministic marker placement produced by richtext.HTMLToMarkdown, reducing accidental rewrites of user-authored text that happens to look like an attachment marker.

Changes:

  • Require attachment marker lines (📎 filename) to be preceded by an empty line (or be at the start) before injecting file sizes.
  • Update the duplicate-filename test to reflect blank-line-separated markers.
  • Add a regression test ensuring user-authored standalone 📎 filename lines following prose are not rewritten.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/commands/chat.go Adds a “preceded by empty line (or start)” guard to prevent unintended size injection into user-authored content.
internal/commands/chat_test.go Updates/extends unit tests to cover the new marker matching rule and the user-authored line regression.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88d080f395

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The empty-preceding-line check blocked valid inline attachments where
HTMLToMarkdown produces "content\n📎 filename\n" (marker follows
non-empty text). The filename-match against the attachment array is
sufficient protection against false positives.
@github-actions github-actions bot added the bug Something isn't working label Mar 16, 2026
@jeremy jeremy merged commit 4a091a2 into main Mar 16, 2026
26 checks passed
@jeremy jeremy deleted the fix-chat-attachment-display branch March 16, 2026 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working commands CLI command implementations tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants