Skip to content

Show the reason for each spelling suggestion in the summary - #115

Merged
lzm0 merged 1 commit into
mainfrom
spell-check-summary-reason
Sep 10, 2026
Merged

lzm0 merged 1 commit into
mainfrom
spell-check-summary-reason

Conversation

@lzm0

@lzm0 lzm0 commented Sep 10, 2026

Copy link
Copy Markdown
Member

What changed

grounded already required every finding to carry a non-empty reason, but it dropped the value on the floor. It now keeps the stripped reason alongside the rest of the finding, and the ## Spelling step summary gains a Reason column.

Why

The summary table listed file, misspelling, and correction, which tells a reviewer what is being suggested but not why it is right. The reason is exactly that justification, and it was already being collected and validated — surfacing it means the table stands on its own without opening the diff or the inline comments.

Notes for the reviewer

  • Review comment bodies are unchanged; the comment comprehension just skips the new tuple element with _.
  • Reasons are model-written free text, so the cell collapses whitespace (" ".join(reason.split())) and escapes | to \| — otherwise a newline or pipe in a reason would break the markdown row.

Findings must already justify themselves, but the reason was only
validated and then discarded. Carry it through to the step summary so
the table explains why each correction is right without opening the
diff. Reasons are free text, so collapse whitespace and escape pipes to
keep each row intact.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
for path, number, word, correction, reason, _ in kept:
link = f"{repository}/blame/{os.environ['HEAD_SHA']}/{path}#L{number}"
print(f"[{Path(path).name}:{number}]({link})|`{word}`|`{correction}`", file=table)
cell = " ".join(reason.split()).replace("|", "\\|")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have to worry about:

File Misspelling Correction Reason
test.txt teh the

something

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You mean html injection?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What would be a bad example?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean, it could produce:

File Misspelling Correction Reason
test.txt teh the

hi

lo

test.txt teeh teeth

mo

go

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why should we worry about this? GitHub is making sure JavaScript injection does not happen and that should be enough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's more about if the model goes wonky the outputs will be overly tall, but whatever.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd expect the output to be plain text. I will adjust the prompt if otherwise.

@lzm0
lzm0 merged commit 67b37b7 into main Sep 10, 2026
5 checks passed
@lzm0
lzm0 deleted the spell-check-summary-reason branch September 10, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants