Skip to content

Remove redundant pr_comment set_status action - #1043

Merged
thisrohangupta merged 2 commits into
harness:mainfrom
SrikarMannepalli:fix/remove-pr-comment-set-status
Oct 4, 2026
Merged

thisrohangupta merged 2 commits into
harness:mainfrom
SrikarMannepalli:fix/remove-pr-comment-set-status

Conversation

@SrikarMannepalli

Copy link
Copy Markdown
Contributor

Summary

  • Removes the set_status execute action from pr_comment. It duplicated resolve and unresolve, which hit the same status endpoint and need no body.
  • Removes the enum field from BodyFieldSpec, which only set_status used, and restores the related test comments.
  • Updates server instructions, resource hints, README and the test plan to describe only resolve / unresolve.

Test plan

  • tsc --noEmit
  • vitest run (only pre-existing ip-address-security-lib failures from the installed dependency version)

Made with Cursor

resolve and unresolve already cover changing a PR comment thread's status
without a body, so set_status duplicated them. Also drops the BodyFieldSpec
enum field that only set_status used.

Co-authored-by: Cursor <cursoragent@cursor.com>

@vivek-kumar-harness vivek-kumar-harness left a comment

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.

The cleanup makes sense: set_status duplicates resolve/unresolve, and keeping the two zero-body actions gives agents a simpler contract. I also verified the affected tests, typecheck, standards checks, and docs check pass.

Two small suggestions:

  1. tests/tools/tool-handlers.test.ts now has two tests for the same resource_id → comment_id behavior (around lines 1690 and 2376). Please keep the existing test at 1690, add the { status: "resolved" } body assertion from the later test, and remove the duplicate.

  2. Consider leaving BodyFieldSpec.enum in place. Although only set_status currently uses it, it is useful agent-facing metadata exposed through harness_describe, and removing it is separate from removing the redundant action. If we intentionally want to remove it as unused API surface, that would be clearer as a separate change.

The remaining README, server-instruction, test-plan, and set_status cleanup looks complete.

Co-authored-by: Cursor <cursoragent@cursor.com>
@SrikarMannepalli

Copy link
Copy Markdown
Contributor Author

Thanks for the review.

  • Test consolidation: done. The resource_id → comment_id test now lives only in the existing harness_execute — PR comment block and also asserts the { status: "resolved" } body. The duplicate is removed.
  • BodyFieldSpec.enum: I'm keeping it removed here. set_status was its only consumer, so it would be unused API surface with no coverage once that action is gone. If we want enum metadata for body fields later, it is easy to reintroduce alongside a real use.

@thisrohangupta
thisrohangupta merged commit cbbd76d into harness:main Oct 4, 2026
8 checks passed
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.

3 participants