Skip to content

exp(collapse): collapsed details review with spoiler-free summary - #6

Open
mmiermans wants to merge 1 commit into
masterfrom
exp/collapse
Open

exp(collapse): collapsed details review with spoiler-free summary#6
mmiermans wants to merge 1 commit into
masterfrom
exp/collapse

Conversation

@mmiermans

Copy link
Copy Markdown
Owner

Experiment PR for collapse. Do not merge.

@mmiermans

Copy link
Copy Markdown
Owner Author

Manual rendering demo (posted by hand to show how the collapse delivery looks on GitHub). In production this body would be authored by the bot.

🤖 Claude has reviewed this PR — expand after you've formed your own opinion

Claude review

  • Bug — webclient/resources/js/exp-collapse.js:3: if (value = 2048) assigns instead of compares; it's always truthy and overwrites value. Use ===.
  • Nit: consider a unit test for clampTile boundary values.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Claude has reviewed this PR - expand after you have formed your own opinion

Summary

This PR rewrites the Claude PR review workflow and adds a small JS helper.

webclient/resources/js/exp-collapse.js

Bug (confirmed): if (value = 2048) uses assignment (=) instead of comparison (===). The comment even flags it as intentional. Effects:

  • The condition is always truthy (assigns 2048, which is truthy), so the function always returns 2048 regardless of input.
  • value is mutated as a side effect.

Fix:

function clampTile(value) {
  if (value === 2048) {
    return 2048;
  }
  return value;
}

Note: clampTile is not referenced anywhere else in the repo, so this file appears to be dead code added only to exercise the review action. Consider removing it before merge if that is the case.

.github/workflows/claude-review.yml

  • Removed prompt-injection mitigations. The previous config used include_comments_by_actor: "mmiermans" and deliberately excluded gh pr view so untrusted PR comments could not reach the model context. The new prompt only reads gh pr diff, which is good, but the diff itself is still attacker-controlled on PRs, so the inlined instructions (post exactly one comment, tool allowlist) are now the main guardrail. Worth confirming this is an acceptable tradeoff.
  • Permissions changed: dropped id-token: write (no longer using OIDC, now auth via github_token + anthropic_api_key) and added issues: write. issues: write is broader than needed for PR comments; pull-requests: write already covers gh pr comment. Consider dropping issues: write unless required.
  • Lost features: use_sticky_comment (updated one comment per push) and track_progress are gone. With concurrency.cancel-in-progress retained, each new push now creates a NEW comment rather than updating a sticky one. If intended, fine; otherwise PRs will accumulate review comments.
  • Minor: the blank-line removals are cosmetic and reduce readability of the section comments, but no functional impact.

Verdict

The JS file contains a real (self-described) bug. The workflow changes are functional but trade away some prompt-injection hardening and sticky-comment behavior; verify those are intentional.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Claude has reviewed this PR - expand after you have formed your own opinion

Summary

This PR rewrites the Claude PR-review GitHub Action and adds a small JS helper.

webclient/resources/js/exp-collapse.js

  • Bug (confirmed): assignment instead of comparison. Line 3 is if (value = 2048). This assigns 2048 to value, so the condition is always truthy and the function always returns 2048. The comment even flags it as intentional. Should be if (value === 2048) — or, given the apparent intent (clamp to a max of 2048), if (value >= 2048).
  • Dead code: clampTile is not referenced anywhere in the codebase. If it is only a fixture to exercise the review action, consider keeping it out of resources/js (which presumably ships to clients) or removing it before merge.
  • Minor: a strict-mode context would turn this kind of accidental assignment into a thrown error — worth considering for these resource scripts.

.github/workflows/claude-review.yml

  • Permissions: id-token: write was removed and issues: write added. Removing OIDC is fine since auth now uses anthropic_api_key + github_token. issues: write is broader than needed for PR review comments (pull-requests: write already covers gh pr comment); drop it unless something explicitly requires it.
  • Prompt-injection posture regressed. The previous config used include_comments_by_actor set to mmiermans and deliberately excluded fetching PR comments. The new prompt only reads gh pr diff, which is safer than reading comments — but the diff itself is attacker-controlled on PRs and is now fed directly into the prompt. The same-repo if: guard is retained, so risk is limited, but the explicit injection mitigations are gone.
  • Allowed tools narrowed to gh pr diff / gh pr comment only — reasonable and tighter than before. Note the inline-comment tool and git log access were dropped, so commit-message context and inline comments are no longer available (intentional given the new one-comment, no-inline rule).
  • Removed comments/blank lines reduce workflow readability but are cosmetic.

Verdict

The intentional = vs === bug is the one real correctness issue. The workflow changes are functional; main call-outs are the unnecessary issues: write scope and the reduced prompt-injection hardening.

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.

1 participant