feat: sandcastle reworks PRs with requested changes #39

Merged

weiwen merged 2 commits from sandcastle/issue-21 into main 2026-07-05 19:03:13 +08:00

Conversation 0 Commits 2 Files changed 5 +223 -39

weiwen commented 2026-07-05 18:58:21 +08:00

Owner

Copy link

Closes #21

What changed and why

The sandcastle planner previously only handled CI-failure rework on open PRs. This adds a second rework path: when a human reviewer submits a REQUEST_CHANGES review, the planner picks up the PR and the pr-fixer addresses the feedback, then re-queues it for human review.

New Forgejo Actions workflow — .github/workflows/pr-triage.yml

Forgejo Actions does not support the pull_request_review trigger available in GitHub Actions. The workflow fires on on: pull_request instead (which also fires on review comments), then queries the reviews API directly to check whether the latest non-COMMENT/non-PENDING review state is REQUEST_CHANGES. When it is, it adds ready-for-agent and removes ready-for-human from the PR. It uses the automatic runner token and falls back to a FORGEJO_TOKEN Actions secret if label-write permissions are insufficient.

Plan schema and orchestrator — .sandcastle/main.mts + plan-prompt.md

The planSchema and PrReworkItem type now include a reworkReason field ("ci-failure" | "review-requested-changes"). The planner query in plan-prompt.md uses a jq expression to separate the two signals: a PR with ready-for-agent is review-requested-changes (takes priority); a PR with ci == "failure" and no ready-for-agent is ci-failure. The REWORK_REASON env var is passed into each pr-fixer sandbox.

PR-fixer prompt — .sandcastle/pr-fixer-prompt.md

The prompt is now branched on {{REWORK_REASON}}:

• ci-failure: unchanged behaviour — reproduce, fix, push, comment; no relabelling.
• review-requested-changes: read the reviews API + inline comments, address the feedback, run the check gate, push, then relabel ready-for-agent → ready-for-human and post a summary comment. The relabel is mandatory to prevent the planner from re-picking the PR.

Also fixed the tea comment command syntax to use the valid tea comment subcommand (not tea comment create).

Docs — docs/agents/triage-labels.md

Updated to document both rework paths, the pr-triage workflow design rationale (Forgejo pull_request_review limitation), label-edit authentication, and the accepted limitation for reviews with no comment thread.

Key decisions

• on: pull_request as a substitute for pull_request_review: This is the only workable Forgejo trigger for review events. The reviews-API guard ensures the workflow only acts on actual REQUEST_CHANGES events.
• ready-for-agent as the signal: Using a label (set by automation) rather than querying the reviews API from the planner keeps the planner query simple (tea pulls list) and avoids a per-PR API call in the planning phase.
• Review path takes priority over CI: If a PR has both signals, ready-for-agent wins. This is correct because a human has explicitly requested changes, and the CI may be failing for the same reason.

Reviewer checklist

• pr-triage.yml: does the jq filter correctly identify REQUEST_CHANGES vs COMMENT/PENDING/APPROVED?
• pr-triage.yml: does the label-resolve + add/delete API call sequence look correct for Forgejo's labels API?
• plan-prompt.md: does the jq expression correctly separate ci-failure from review-requested-changes when both signals are present?
• pr-fixer-prompt.md: are the tea comment and tea pulls review-comments commands valid for the version of tea in use?
• Accepted limitation (review with no comment thread may not fire pull_request) is correctly documented and acceptable.

Closes #21 ## What changed and why The sandcastle planner previously only handled CI-failure rework on open PRs. This adds a second rework path: when a human reviewer submits a REQUEST_CHANGES review, the planner picks up the PR and the pr-fixer addresses the feedback, then re-queues it for human review. ### New Forgejo Actions workflow — `.github/workflows/pr-triage.yml` Forgejo Actions does not support the `pull_request_review` trigger available in GitHub Actions. The workflow fires on `on: pull_request` instead (which also fires on review comments), then queries the reviews API directly to check whether the latest non-COMMENT/non-PENDING review state is `REQUEST_CHANGES`. When it is, it adds `ready-for-agent` and removes `ready-for-human` from the PR. It uses the automatic runner token and falls back to a `FORGEJO_TOKEN` Actions secret if label-write permissions are insufficient. ### Plan schema and orchestrator — `.sandcastle/main.mts` + `plan-prompt.md` The `planSchema` and `PrReworkItem` type now include a `reworkReason` field (`"ci-failure" | "review-requested-changes"`). The planner query in `plan-prompt.md` uses a `jq` expression to separate the two signals: a PR with `ready-for-agent` is `review-requested-changes` (takes priority); a PR with `ci == "failure"` and no `ready-for-agent` is `ci-failure`. The `REWORK_REASON` env var is passed into each pr-fixer sandbox. ### PR-fixer prompt — `.sandcastle/pr-fixer-prompt.md` The prompt is now branched on `{{REWORK_REASON}}`: - **`ci-failure`**: unchanged behaviour — reproduce, fix, push, comment; no relabelling. - **`review-requested-changes`**: read the reviews API + inline comments, address the feedback, run the check gate, push, then relabel `ready-for-agent → ready-for-human` and post a summary comment. The relabel is mandatory to prevent the planner from re-picking the PR. Also fixed the `tea comment` command syntax to use the valid `tea comment` subcommand (not `tea comment create`). ### Docs — `docs/agents/triage-labels.md` Updated to document both rework paths, the pr-triage workflow design rationale (Forgejo `pull_request_review` limitation), label-edit authentication, and the accepted limitation for reviews with no comment thread. ## Key decisions - **`on: pull_request` as a substitute for `pull_request_review`**: This is the only workable Forgejo trigger for review events. The reviews-API guard ensures the workflow only acts on actual REQUEST_CHANGES events. - **`ready-for-agent` as the signal**: Using a label (set by automation) rather than querying the reviews API from the planner keeps the planner query simple (`tea pulls list`) and avoids a per-PR API call in the planning phase. - **Review path takes priority over CI**: If a PR has both signals, `ready-for-agent` wins. This is correct because a human has explicitly requested changes, and the CI may be failing for the same reason. ## Reviewer checklist - [ ] `pr-triage.yml`: does the `jq` filter correctly identify REQUEST_CHANGES vs COMMENT/PENDING/APPROVED? - [ ] `pr-triage.yml`: does the label-resolve + add/delete API call sequence look correct for Forgejo's labels API? - [ ] `plan-prompt.md`: does the `jq` expression correctly separate `ci-failure` from `review-requested-changes` when both signals are present? - [ ] `pr-fixer-prompt.md`: are the `tea comment` and `tea pulls review-comments` commands valid for the version of `tea` in use? - [ ] Accepted limitation (review with no comment thread may not fire `pull_request`) is correctly documented and acceptable.

weiwen added the

ready-for-human

label 2026-07-05 18:58:21 +08:00

weiwen added 2 commits 2026-07-05 18:58:21 +08:00

RALPH: feat: sandcastle reworks PRs with requested changes (closes #21) ... 383842a5ff

Task: Add review-driven PR-rework signal on top of CI-fail path (#20).

Key decisions:
- pr-triage.yml workflow triggers on `pull_request` (Forgejo's substitute
  for the unsupported `pull_request_review` event) and queries the reviews
  API to guard against non-review events. Only fires on REQUEST_CHANGES.
  Auth: automatic GITHUB_TOKEN, falls back to FORGEJO_TOKEN secret.
- plan-prompt.md: combined jq query emits both CI-failure and
  review-requested-changes items with a `reworkReason` field.
  If a PR has both signals, ready-for-agent (review) takes priority.
- planSchema / PrReworkItem gain reworkReason: z.enum([...]).
- pr-fixer receives REWORK_REASON promptArg; prompt branches on it:
  ci-failure path = existing behaviour (no relabel);
  review path = read reviews, address feedback, push, relabel
  ready-for-agent → ready-for-human, post summary comment.
- docs/agents/triage-labels.md documents both rework paths, the
  pull_request-trigger + reviews-API-check mechanism, auth approach,
  and the accepted limitation for comment-free reviews.

Files changed:
- .github/workflows/pr-triage.yml (new): Forgejo Actions workflow
- .sandcastle/plan-prompt.md: unified rework-prs query with reworkReason
- .sandcastle/main.mts: schema, type, promptArgs, header comment
- .sandcastle/pr-fixer-prompt.md: two-branch prompt (ci vs review)
- docs/agents/triage-labels.md: review-driven rework path documented

Blockers/notes:
- No Rust toolchain in sandbox; Rust tests must be validated by CI.
- The sandcastle TypeScript changes are not exercised by `just check`
  (no TS test suite); correctness was verified by jq dry-run and code
  review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix: use valid tea commands in pr-fixer rework prompt ... 4918254b27

The review-requested-changes branch of the pr-fixer prompt instructed the
agent to run subcommands and flags that do not exist in the tea CLI
(verified against tea 0.14.2, the version the repo standardizes on):

- `tea pr reviews` — no such subcommand (only `review`/`review-comments`).
  Replaced with the reviews API (`GET /repos/.../pulls/<n>/reviews`), which
  returns each review's state and summary body — the same endpoint the
  pr-triage workflow uses.
- `tea pr edit --add-label/--remove-label` — invalid flags. tea uses
  `--add-labels`/`--remove-labels` (plural), as in pr-prompt.md.
- `tea comment create ... --body` — no `create` subcommand and no `--body`
  flag; the documented shorthand is `tea comment <index> "<body>"`
  (docs/agents/issue-tracker.md). Fixed both occurrences.

Also switched `tea pr review-comments` → `tea pulls review-comments` for
consistency with the repo's use of the `pulls` noun, and refreshed two
stale inline comments in main.mts that still described CI as the only
rework signal.

No behavioural change to main.mts logic — comment-only edits there.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

weiwen merged commit a8bdcdc8cd into main 2026-07-05 19:03:13 +08:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

in-review

Agent opened a PR; tracked there — no action needed on the issue

  

ready-for-agent

PRD ready for an agent to implement

  

ready-for-human

Implemented; awaiting human review/merge

No labels

in-review

ready-for-agent

ready-for-human

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

weiwen/evie!39

No description provided.