Skip to content

feat(#1654): scope review to own commits on stacked/cherry-picked PRs - #1661

Open
fullsend-ai-coder[bot] wants to merge 17 commits into
mainfrom
agent/1654-scope-stacked-pr-review
Open

fullsend-ai-coder[bot] wants to merge 17 commits into
mainfrom
agent/1654-scope-stacked-pr-review

Conversation

@fullsend-ai-coder

@fullsend-ai-coder fullsend-ai-coder Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The review agent reviewed the full base-vs-head diff. When a PR branch carried commits from another unmerged PR (a stacked branch, or commits cherry-picked from a dependency), it raised false-positive findings on code the author did not write in that PR.

This adds a dependency-scoping step to the pr-review skill:

  • skills/pr-review/SKILL.md: new step 2c pointing to the procedure, which runs before step 2a and dispatch.
  • skills/pr-review/references/dependency-scope.md (new): the procedure.
    • Detection: only PRs that the PR author names as a dependency in the PR body or their own comments ("depends on #N", "stacked on #N", "cherry-picked from #N", ...), in the same repo. That text is untrusted. It only chooses which PR to compare against.
    • Matching: a PR commit counts as a dependency commit only if it shares a SHA with an open candidate PR's commits (stacked) or has an equal git patch-id --stable (cherry-pick). A matching commit message alone is not enough, so a cherry-pick whose conflicts were resolved differently stays in scope.
    • Applying the scope: sub-agents review pr-diff.txt, which now holds only the PR's own commits. The full diff stays in pr-full-diff.txt as context. Every context package gets a dependency_scope note, and step 6e drops findings on code that dependency commits introduced.
    • Safety rails: protected-path and PR body checks still cover the whole PR. Dependency scoping does not impose merge ordering or change normal approval criteria; the review body states which commits were excluded.
  • skills/pr-review/github/SKILL.md: new "Dependency-PR scoping" command. It narrows the diff only when the dependency commits come first and are followed by own commits, there are no merge commits, both PRs have fewer than 250 commits, and the compare from the last dependency commit to HEAD is complete. Otherwise it prints SKIP: <reason> and the full diff is reviewed as before.

Caveats

  • pr-review/SKILL.md is at its skillsaw context-budget ceiling (17,036 tokens). To keep the new pointer token-neutral, I removed the duplicated "If no PR can be identified, stop..." sentence at the end of step 1. The same instruction is still in step 1's list.
  • GitLab is not covered: there is no GitLab command, so GitLab MRs still get a full-diff review.
  • Undeclared dependencies are not detected: the agent does not search other open PRs for matching commits.
  • Interleaved commits fall back to a full review: if own commits and dependency commits are mixed, the narrowing is skipped.

Testing

  • scripts/pr-review-dependency-scope-test.sh (new, added to make script-test) runs the forge snippet against a stubbed gh in seven cases. Two should narrow the diff: cherry-picked commits and a stacked branch. Five should fall back to the full diff: a dependency PR that is not open, interleaved commits, a merge commit, a cherry-pick with a different patch, and an incomplete compare. A mutation that disables patch-id matching makes the cherry-pick case fail.
  • Ran the snippet once against the live GitHub API (fix(review): gate re-review severity transitions #1636) to check the commit-list, patch-id and compare response shapes.
  • skillsaw --strict passes. pr-review-remediation-test.sh, pr-review-challenger-contract-test.sh and risk-tier1-test.sh pass.
  • pre-commit could not fetch its hook repos in the sandbox (TLS failure). I ran the pre-commit-hooks checks and shellcheck directly at their pinned revs, and they pass. scan-secrets passes.

🤖 Generated with Claude Code


Closes #1654

Post-script verification

  • Branch is not main/master (agent/1654-scope-stacked-pr-review)
  • Secret scan passed (gitleaks — d8a9bc78b9566158a03efda6265d904c7dfc1981..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

Approval-policy clarification (2026-10-12)

Dependency scoping is evidence selection, not merge-order enforcement. A verified open dependency may establish that a prefix of commits belongs elsewhere, but it does not prove the dependency must merge first or that the child cannot be approved independently. The review body identifies excluded commits and the source PR; normal approval criteria still apply.

Post-change verification

  • Producer/consumer matrix — verified: GitHub accepts only author-declared, same-repository open candidate PRs whose leading commits match by SHA or whitespace-preserving patch equivalence; closed, unmatched, interleaved, merge, unavailable, moved-head, unsafe-path, or incomplete-compare cases are rejected and fall back to the full diff. GitLab has no dependency-scoping command, so it rejects scoping and always uses the full diff. Neither forge treats an open dependency as an approval veto.
  • Instruction surfaces — verified: the review-agent approval contract, dependency-scoping procedure, user documentation, and executable dependency-scope contract test now agree.
  • Adversarial re-review — verified: the executable test models a dependency prefix followed by a direct remediation in a dependency-touched file plus an unrelated file edit; the scoped incremental diff retains both own changes.
  • Historical constraint — verified: this preserves Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654's false-positive prevention while removing the extra merge-order policy identified by the review.
  • Behavioral validation — verified: bash scripts/pr-review-dependency-scope-test.sh, make script-test, and staged make lint passed.

Re-review fixes (2026-10-12)

  • Producer/consumer matrix — verified: GitHub security dispatch and security context now consume the full PR diff and full changed-file list whenever own-commit scoping succeeds; other dimensions consume the scoped own-commit artifacts. GitHub accepts only positive-decimal dependency IDs; malformed candidates and failed raw-compare retrieval fall back to the full diff. GitLab has no dependency-scoping command and continues to review the full diff.
  • Instruction surfaces — verified: the orchestrator context-package rule, large-PR security context rule, dependency-scope procedure, GitHub forge command, user documentation, and executable dependency-scope suite now agree.
  • Adversarial coverage — verified: the suite exercises a security full-context contract, a wildcard dependency candidate (must fall back), and an own commit combining content plus an executable-bit change (must retain mode metadata). Existing remediation-plus-unrelated-edit coverage remains.
  • Historical/security constraint — verified: a matching open dependency is evidence only of attribution, never security clearance; security findings and inspection cover all commits that the PR would merge.
  • Behavioral validation — verified: bash scripts/pr-review-dependency-scope-test.sh, make script-test, staged make lint, and an independent re-review passed.

Final-newline review fix (2026-10-18)

  • Producer/consumer matrix — verified: GitHub accepts shared SHAs or cherry-picks with both matching stable patch IDs and exact hashes of changed lines plus Git final-newline markers. A newline-only mismatch is classified as an own commit; when the required dependency-prefix shape is absent, SKIP keeps the full PR diff. Fetch, shape, and compare failures also keep the full diff. GitLab has no scoping command and always reviews the full diff. Forgejo has no review harness in this repository, so it is not applicable.
  • Instruction surfaces — verified: Updated the GitHub command and the dependency-scoping reference. The orchestrator's dispatch/context rules, user guide, review prompt, and GitLab command remain consistent: security and protected-path inspection use the full PR, while GitHub non-security dimensions receive a scoped diff only after SCOPED.
  • Adversarial re-review — verified: The executable fixture includes a direct remediation, another unrelated edit inside the remediated file, and an unrelated file; all remain in the scoped incremental diff.
  • Historical constraints — verified: Issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654's false-positive reduction remains for identical dependency commits. The existing whole-PR security inspection and conservative fallback remain intact.
  • Behavioral validation — verified locally: The new final-newline case failed against the old matcher (SCOPED) and passes with the fix (SKIP). The dependency-scope, re-review remediation, and challenger contract suites, staged make lint, pre-commit, and diff checks passed. A live stacked-PR end-to-end run has not been performed. Current-head script-test CI passes; automated review remains in progress.
  • Optional complexity finding — deferred: The current checks enforce attribution and safe fallback. Moving the command into a separately invoked script would add packaging and call-path work without fixing this bug; the maintainer prefers the focused fix.
  • Protected-path approval — approval required: This PR still needs independent human approval for its protected skill and script paths before merge.

Current-main CI integration (2026-10-18)

Current main added scripts/skill-env-reachability-test.sh after this PR's earlier CI run. The CI merge flagged 14 scoping-command inputs, local variables, and the literal SCOPED as missing sandbox environment values. This is a scanner classification issue: the command takes DEP_PRS and PINNED_SHA in its own Bash call, and its other flagged values are assigned locally. Merged current main and classified those values without adding sandbox credentials or changing forge behavior.

  • Producer/consumer matrix — verified: GitHub command inputs and its SCOPED/SKIP outputs are unchanged; GitLab keeps full-diff review; Forgejo remains not applicable to this harness.
  • Instruction surfaces — verified: the scanner allowlist and GitHub command assignments agree. Dispatch, context packages, user guidance, and security/protected-path rules are unchanged.
  • Adversarial re-review — verified: the direct-remediation, same-file unrelated edit, and unrelated-file fixture still passes after the merge.
  • Historical constraints — verified: preserves Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654's safe attribution and the scanner's test(scripts): add skill env var reachability test #1665 credential-reachability purpose; no credential is newly declared or passed to the sandbox.
  • Behavioral validation — verified locally: the scanner failed in the same 14 places as CI before the integration change and passed after it. The dependency-scoping suite, staged skillsaw lint, pre-commit, and diff checks passed on the merged tree. The full script-test CI rerun passed on 0ed6e55; automated review remains in progress.

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner October 12, 2026 07:20
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Oct 12, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:21 AM UTC · Completed 7:33 AM UTC

Commit: 1f1ffe7 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.60

@fullsend-ai-review fullsend-ai-review Bot added the risk/elevated PR risk: elevated label Oct 12, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Score remains at 3 (elevated) as PR files are unchanged from the prior review, with risk driven by protected path modifications, workflow changes, and high Tier 2 churn balanced by a well-scoped linked issue.

Previous run

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Score stays at the prior 3 (elevated). Tier 1 is about 2.6 (9 files, 766 lines, large blast radius, protected paths, workflow change, bot author, no dependency changes), matching the prior run. Tier 2 is high (about 3.7) from churn and fix history, and Tier 3 is low (about 1.8) because the issue is well scoped. The weighted composite is about 2.8, which rounds to 3.

Previous run (2)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Score stays at the prior 3 (elevated) because Tier 1 is unchanged (8 files, 736 lines, 5 protected paths, CI/workflow change, bot author, no dependency changes, Tier 1 about 2.6). Tier 2 is high (about 3.7) from heavy churn, many authors and many fix commits on the touched files. Tier 3 is low (about 1.8) since issue #1654 is well scoped. The weighted composite is about 2.8, which rounds to 3.

Previous run (3)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Score stays at the prior 3 (elevated). Tier 1 is unchanged: 8 files, about 730 changed lines, 5 protected paths, bot author and no dependency changes. The only change since the prior assessment is a merge of main. Tier 2 is high because of heavy churn, many authors and many fix commits. Tier 3 is low because issue #1654 is well scoped. The weighted composite is about 2.9, which rounds to 3.

Previous run (4)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Tier 1 signals are unchanged from the prior run (8 files, 5 protected paths, bot author, no dependency changes). Tier 1 is about 2.6, Tier 2 is about 4 because of heavy churn, many authors and many fix commits, and Tier 3 is about 2. The weighted composite is about 2.9, so the score stays at the prior 3.

Previous run (5)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

The Tier 1 signals are unchanged from the prior run: a similar file count and size, the same protected paths, and a bot author. Tier 2 shows heavy churn, many authors and many fix commits in the touched files, and Tier 3 is low because the issue is well scoped. The weighted composite is about 2.6, so the score stays at the prior 3.

Previous run (6)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

The PR is large (718 lines) and touches 5 protected paths and CI-related files, in heavily churned, fix-heavy files (agents/review.md, skills/pr-review/SKILL.md, docs/review.md). It has no feature flag, which gives a Tier 1 score of 2.63, Tier 2 of 3.14 and Tier 3 of 1.67. The bot author, no dependency or security-sensitive changes, a new test script and a well-scoped linked issue offset some of that. The weighted composite is about 2.6, which rounds to 3 and matches the prior score of 3 because the signals are unchanged.

Previous run (7)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

A large, 734-line change touching 6 protected paths and CI-related files, in frequently modified, fix-heavy files, with no feature flag, is offset by a bot author, no dependency or security-sensitive changes, a new test script, and a well-scoped linked issue. The weighted composite is about 2.7, which rounds to 3.

Previous run (8)

<!-- fullsend:risk-assessment -->
Risk Assessment: elevated (3/5)

Details

Tier 1 is about 2.6 (large blast radius, 6 protected paths, CI/workflow flag, low test ratio, bot author, no dependency or security files), essentially unchanged from the prior run, and Tier 2 (heavily churned, multi-author files) is about 3.3 and Tier 3 (additive change, no feature flag) is about 3. The composite is about 2.9, so the prior score of 3 is kept.

Previous run (9)

Risk Assessment: elevated (3/5)

Details

Tier 1 signals are unchanged from the prior assessment, so the prior score of 3 is kept. Tier 1 is about 2.6 (large blast radius, 5 protected paths, CI/workflow flag, low test ratio, bot author, no dependency or security files). Tier 2 is about 3.0 (heavily churned, multi-author files). Tier 3 is about 3 (additive change with no feature flag). The composite is about 2.8, which rounds to 3.

Previous run (10)

Risk Assessment: elevated (3/5)

Details

The composite is about 2.8, which rounds to 3. Tier 1 is about 2.6, Tier 2 about 3.0 and Tier 3 about 3. Five protected paths, a large blast radius, a CI/workflow flag, and heavily churned multi-author files raise the score. Bot authorship, no dependency or security-sensitive files, and mostly additive changes keep it from going higher. Tier 1 signals match the prior assessment, so the prior score of 3 is kept.

Previous run (11)

Risk Assessment: elevated (3/5)

Details

The composite is about 3.0 (Tier 1 about 2.6, Tier 2 about 3.7, Tier 3 about 3). Several protected paths, a large blast radius and heavily churned multi-author files drive the score. Bot authorship, no security-sensitive or dependency files, and mostly additive scope hold it down. Tier 1 signals still match the prior assessment (score 3), so that score is kept.

Previous run (12)

Risk Assessment: elevated (3/5)

Details

The composite is about 3.0 (Tier 1 about 2.6, Tier 2 about 3.7, Tier 3 about 3); 5 protected paths, a large blast radius and heavily churned multi-author files drive it, while bot authorship, no security-sensitive or dependency files, and mostly additive scope hold it down, and Tier 1 signals match the prior review so the prior score of 3 is kept.

Previous run (13)

Risk Assessment: elevated (3/5)

Details

Tier 1 signals are unchanged from the prior review (7 files, 5 protected paths, CI-flagged path, large blast radius, no feature flag, high-churn multi-author files). Lines changed rose slightly to 598, still in the 300-799 band. The weighted composite is about 2.9, so the prior elevated score of 3 is kept.

Previous run (14)

Risk Assessment: elevated (3/5)

Details

Tier 1 signals are unchanged from the prior review (7 files, 5 protected paths, CI-flagged path, large blast radius, no feature flag, high-churn multi-author files); the extra commit moved lines changed from about 489 to 587, within the same band, so the prior elevated score is kept.

Previous run (15)

Risk Assessment: elevated (3/5)

Details

Weighted composite is about 2.7 (Tier 1 2.63, Tier 2 3.7, Tier 3 1.5), which rounds to 3. Tier 1 characteristics are unchanged from the prior review, so the prior elevated score is kept: same 7 files, 5 protected paths, a CI-flagged path, no feature flag, and high-churn multi-author files.

Previous run (16)

Risk Assessment: elevated (3/5)

Details

Computed composite is about 2.4, but Tier 1 characteristics are unchanged from the prior review (same 7 files, 437 to 489 lines, still medium blast radius, 5 protected paths, CI-flagged path, no feature flag, very high-churn multi-author files), and Tier 2/3 give no specific reason to lower it, so the prior elevated score of 3 is preserved.

Previous run (17)

Risk Assessment: elevated (3/5)

Details

Medium-size change (7 files, 437 lines) by a bot that touches 5 protected paths and a CI/workflow-flagged path, modifies existing review-skill behavior with no feature flag, and edits very high-churn, multi-author files, offset by an in-scope linked issue and no dependency or security-sensitive changes.

Previous run (18)

Risk Assessment: elevated (3/5)

Details

Small additive PR, but it touches protected paths, the files have high churn and many authors, and it changes review-agent behavior with no feature flag.

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected governance/infrastructure paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, scripts/skill-env-reachability-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.

Low

  • [logic-error] skills/pr-review/github/SKILL.md:182 — An empty commit disables dependency scoping for an otherwise valid stack. git patch-id --stable produces no output for an empty diff, so ids() fails hash validation and sets SKIP. This also affects dependencies matched by exact SHA, because every dependency commit is hashed before SHA-based classification. A harmless CI-retrigger commit can therefore cause dependency code to receive findings again. The failure is fail-safe (the full diff is reviewed).
    Remediation: Handle successfully fetched empty diffs separately from fetch or hashing failures, and avoid requiring patch IDs for dependency commits already matched by SHA. Add a regression case containing an empty commit.

Earlier findings

  • skills/pr-review/SKILL.md:763, incorrect-doc, resolved by change in an earlier push.
  • skills/pr-review/SKILL.md:1002, incomplete-doc, resolved by change in an earlier push.
  • skills/pr-review/github/SKILL.md:163, logic-error, resolved by change in an earlier push.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzMzMGMwNzU3MTNiMzg4MTEiLCJsaW5lIjoxODIsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6InNjb3BlLWNyZWVwIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2ViZTA0Nzg1OTQ3MmEwYzkiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJkZXNpZ24tc21lbGwiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsImxpbmUiOjE0OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InVuYXV0aG9yaXplZC1jaGFuZ2UiLCJmaWxlIjoiYWdlbnRzL3Jldmlldy5tZCIsImlkIjoiZl85NzAyZjgwYTEyMDUzM2RlIiwibGluZSI6MjU5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibWlzc2luZy1hdXRob3JpemF0aW9uIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzEwMDFkNGIyMTZjYzA2ODciLCJsaW5lIjoxLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiZGVzaWduLWRpcmVjdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl82ZTU5MTZlNzUxOWY0YzAwIiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfMTc0ZDViNTQyMDBkYzczNyIsImxpbmUiOjkyLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiaW5jb3JyZWN0LWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmX2VjNjU5Nzg0NjA2MjYxYTMiLCJsaW5lIjo3NjMsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsImxpbmUiOjEwMDIsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9tZXRhLXByb21wdC5tZCIsImlkIjoiZl85NzE1MjQ1MDI4YjkyMGU1IiwibGluZSI6NCwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfN2I3MzUyMzdiOTkyYzIzNiIsImxpbmUiOjEzNTAsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl83ODAwYTQzMTNhM2U4NWJhIiwibGluZSI6MTYzLCJhY3Rpb25hYmxlIjp0cnVlfV0sImRpc3Bvc2l0aW9ucyI6W3siaWQiOiJmXzMzMGMwNzU3MTNiMzg4MTEiLCJzdGF0dXMiOiJyZWNsYXNzaWZpZWQifSx7ImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzk3MDJmODBhMTIwNTMzZGUiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzk3MTUyNDUwMjhiOTIwZTUiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl83YjczNTIzN2I5OTJjMjM2Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9XX0= -->

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies governance/infrastructure paths (agents/review.md, scripts/pr-review-dependency-scope-test.sh, scripts/skill-env-reachability-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md). It links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes regardless of context.

Low

  • [logic-error] skills/pr-review/github/SKILL.md:182 — The ids() helper can classify different metadata-only commits (pure renames or mode changes with no content lines) as identical dependency commits. EXACT hashes only +/- lines and final-newline markers, so content-free renames all get the empty-input SHA-256, while git patch-id --stable strips whitespace from header lines (renaming a b to c d collides with ab to cd). If such a rename is this PR's leading commit and a dependency PR has the colliding rename, it is marked dep and left out of the scoped diff that non-security dimensions review. Security review still receives the full diff. This code is unchanged since the prior review; the latest push only reformatted nearby statements.
    Remediation: Include whitespace-preserving file-operation metadata (diff --git headers, rename/copy from/to lines, old/new mode lines) in the EXACT hash, or classify commits with no +/- lines as own. Add a regression case with whitespace-colliding rename paths.

Earlier findings

  • skills/pr-review/github/SKILL.md:182, logic-error — reclassified from medium to low: only content-free renames or mode changes can collide, and the security dimension still reviews the full PR diff.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzMzMGMwNzU3MTNiMzg4MTEiLCJsaW5lIjoxODEsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6InNjb3BlLWNyZWVwIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2ViZTA0Nzg1OTQ3MmEwYzkiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJkZXNpZ24tc21lbGwiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsImxpbmUiOjE0OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InVuYXV0aG9yaXplZC1jaGFuZ2UiLCJmaWxlIjoiYWdlbnRzL3Jldmlldy5tZCIsImlkIjoiZl85NzAyZjgwYTEyMDUzM2RlIiwibGluZSI6MjU5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibWlzc2luZy1hdXRob3JpemF0aW9uIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzEwMDFkNGIyMTZjYzA2ODciLCJsaW5lIjoxLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiZGVzaWduLWRpcmVjdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl82ZTU5MTZlNzUxOWY0YzAwIiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfMTc0ZDViNTQyMDBkYzczNyIsImxpbmUiOjkyLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiaW5jb3JyZWN0LWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmX2VjNjU5Nzg0NjA2MjYxYTMiLCJsaW5lIjo3NjMsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsImxpbmUiOjEwMDIsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9tZXRhLXByb21wdC5tZCIsImlkIjoiZl85NzE1MjQ1MDI4YjkyMGU1IiwibGluZSI6NCwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfN2I3MzUyMzdiOTkyYzIzNiIsImxpbmUiOjEzNTAsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl83ODAwYTQzMTNhM2U4NWJhIiwibGluZSI6MTYzLCJhY3Rpb25hYmxlIjp0cnVlfV0sImRpc3Bvc2l0aW9ucyI6W3siaWQiOiJmX2ViZTA0Nzg1OTQ3MmEwYzkiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl9kNzI2M2Y2OTA5NTE4MjMwIiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfOTcwMmY4MGExMjA1MzNkZSIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzEwMDFkNGIyMTZjYzA2ODciLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl82ZTU5MTZlNzUxOWY0YzAwIiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfMTc0ZDViNTQyMDBkYzczNyIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmX2VjNjU5Nzg0NjA2MjYxYTMiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl84MjEwNWEzOWE0MjY3NzkwIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzdiNzM1MjM3Yjk5MmMyMzYiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl83ODAwYTQzMTNhM2U4NWJhIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn1dfQ== -->

Review

Findings

Medium

  • [logic-error] skills/pr-review/github/SKILL.md:181 — The additional hash does not distinguish identical edits to different repeated blocks in the same file. git patch-id --stable ignores hunk locations and function headers, while EXACT includes only changed lines and file headers. Patches changing return x to return y in different functions, with identical surrounding body lines, produce both the same patch ID and the same EXACT hash. If the dependency changes the first function and this PR's leading commit changes the second, that distinct own commit is classified as dep; with a subsequent own commit, scoping succeeds and excludes the first commit from non-security review even though the dependency PR does not contain it. Security review is unaffected because it still receives the full diff.
    Remediation: Validate the affected target blocks before accepting patch-ID candidates, conservatively falling back to full review when repeated blocks make equivalence ambiguous. Add a regression case to scripts/pr-review-dependency-scope-test.sh where otherwise identical patches affect different functions in the same file.

  • [protected-path] skills/pr-review/SKILL.md — This PR modifies protected governance/infrastructure paths: agents/review.md, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md, and scripts/pr-review-dependency-scope-test.sh. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5IiwibGluZSI6MTQ5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZGVzaWduLXNtZWxsIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2Q3MjYzZjY5MDk1MTgyMzAiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJ1bmF1dGhvcml6ZWQtY2hhbmdlIiwiZmlsZSI6ImFnZW50cy9yZXZpZXcubWQiLCJpZCI6ImZfOTcwMmY4MGExMjA1MzNkZSIsImxpbmUiOjI1OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3IiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImRlc2lnbi1kaXJlY3Rpb24iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsImxpbmUiOjEsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJsaW5lIjo5MiwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9TS0lMTC5tZCIsImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwibGluZSI6NzYzLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzgyMTA1YTM5YTQyNjc3OTAiLCJsaW5lIjoxMDAyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvbWV0YS1wcm9tcHQubWQiLCJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsImxpbmUiOjQsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzdiNzM1MjM3Yjk5MmMyMzYiLCJsaW5lIjoxMzUwLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoibG9naWMtZXJyb3IiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsImxpbmUiOjE2MywiYWN0aW9uYWJsZSI6dHJ1ZX1dLCJkaXNwb3NpdGlvbnMiOlt7ImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzk3MDJmODBhMTIwNTMzZGUiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzk3MTUyNDUwMjhiOTIwZTUiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl83YjczNTIzN2I5OTJjMjM2Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9XX0= -->

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected governance/infrastructure paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.

Labels: The PR adds a new capability (dependency-PR scoping) to the review skill.

Previous run (4)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5IiwibGluZSI6MTQ5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZGVzaWduLXNtZWxsIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2Q3MjYzZjY5MDk1MTgyMzAiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJ1bmF1dGhvcml6ZWQtY2hhbmdlIiwiZmlsZSI6ImFnZW50cy9yZXZpZXcubWQiLCJpZCI6ImZfOTcwMmY4MGExMjA1MzNkZSIsImxpbmUiOjI1OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3IiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImRlc2lnbi1kaXJlY3Rpb24iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsImxpbmUiOjEsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJsaW5lIjo5MiwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9TS0lMTC5tZCIsImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwibGluZSI6NzYzLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzgyMTA1YTM5YTQyNjc3OTAiLCJsaW5lIjoxMDAyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvbWV0YS1wcm9tcHQubWQiLCJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsImxpbmUiOjQsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzdiNzM1MjM3Yjk5MmMyMzYiLCJsaW5lIjoxMzUwLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoibG9naWMtZXJyb3IiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsImxpbmUiOjE2MywiYWN0aW9uYWJsZSI6dHJ1ZX1dLCJkaXNwb3NpdGlvbnMiOlt7ImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzk3MDJmODBhMTIwNTMzZGUiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzk3MTUyNDUwMjhiOTIwZTUiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl83YjczNTIzN2I5OTJjMjM2Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9XX0= -->

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.
    Remediation: A human reviewer with authority over governance and infrastructure files must approve before merge.
Previous run (5)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5IiwibGluZSI6MTQ5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZGVzaWduLXNtZWxsIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2Q3MjYzZjY5MDk1MTgyMzAiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJ1bmF1dGhvcml6ZWQtY2hhbmdlIiwiZmlsZSI6ImFnZW50cy9yZXZpZXcubWQiLCJpZCI6ImZfOTcwMmY4MGExMjA1MzNkZSIsImxpbmUiOjI1OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3IiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImRlc2lnbi1kaXJlY3Rpb24iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsImxpbmUiOjEsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJsaW5lIjo5MiwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9TS0lMTC5tZCIsImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwibGluZSI6NzYzLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzgyMTA1YTM5YTQyNjc3OTAiLCJsaW5lIjoxMDAyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvbWV0YS1wcm9tcHQubWQiLCJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsImxpbmUiOjQsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzdiNzM1MjM3Yjk5MmMyMzYiLCJsaW5lIjoxMzUwLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoibG9naWMtZXJyb3IiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfNzgwMGE0MzEzYTNlODViYSIsImxpbmUiOjE2MywiYWN0aW9uYWJsZSI6dHJ1ZX1dLCJkaXNwb3NpdGlvbnMiOlt7ImlkIjoiZl83ODAwYTQzMTNhM2U4NWJhIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfZWJlMDQ3ODU5NDcyYTBjOSIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmX2Q3MjYzZjY5MDk1MTgyMzAiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl85NzAyZjgwYTEyMDUzM2RlIiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfMTAwMWQ0YjIxNmNjMDY4NyIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzZlNTkxNmU3NTE5ZjRjMDAiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8xNzRkNWI1NDIwMGRjNzM3Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfZWM2NTk3ODQ2MDYyNjFhMyIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzgyMTA1YTM5YTQyNjc3OTAiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl85NzE1MjQ1MDI4YjkyMGU1Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfN2I3MzUyMzdiOTkyYzIzNiIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9XX0= -->

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected governance/infrastructure paths: agents/review.md, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md, and scripts/pr-review-dependency-scope-test.sh. It links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and the description explains the rationale, but human approval is always required for protected-path changes regardless of context.

Earlier findings

  • skills/pr-review/github/SKILL.md:163, logic-error, resolved by change: the latest commit replaces the read -r -a here-string and compound [[ ... =~ ... ]] check in the dependency-scoping command with a for d in ${DEP_PRS:-} loop under set -f and two separate grep -Eqx checks. The new globbed-dependency-number test case fails when set -f is removed.
Previous run (6)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzc4MDBhNDMxM2EzZTg1YmEiLCJsaW5lIjoxNjMsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6InNjb3BlLWNyZWVwIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2ViZTA0Nzg1OTQ3MmEwYzkiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJkZXNpZ24tc21lbGwiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsImxpbmUiOjE0OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InVuYXV0aG9yaXplZC1jaGFuZ2UiLCJmaWxlIjoiYWdlbnRzL3Jldmlldy5tZCIsImlkIjoiZl85NzAyZjgwYTEyMDUzM2RlIiwibGluZSI6MjU5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibWlzc2luZy1hdXRob3JpemF0aW9uIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzEwMDFkNGIyMTZjYzA2ODciLCJsaW5lIjoxLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiZGVzaWduLWRpcmVjdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl82ZTU5MTZlNzUxOWY0YzAwIiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfMTc0ZDViNTQyMDBkYzczNyIsImxpbmUiOjkyLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiaW5jb3JyZWN0LWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmX2VjNjU5Nzg0NjA2MjYxYTMiLCJsaW5lIjo3NjMsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfODIxMDVhMzlhNDI2Nzc5MCIsImxpbmUiOjEwMDIsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9tZXRhLXByb21wdC5tZCIsImlkIjoiZl85NzE1MjQ1MDI4YjkyMGU1IiwibGluZSI6NCwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfN2I3MzUyMzdiOTkyYzIzNiIsImxpbmUiOjEzNTAsImFjdGlvbmFibGUiOmZhbHNlfV0sImRpc3Bvc2l0aW9ucyI6W3siaWQiOiJmX2VjNjU5Nzg0NjA2MjYxYTMiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl84MjEwNWEzOWE0MjY3NzkwIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzdiNzM1MjM3Yjk5MmMyMzYiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfZDcyNjNmNjkwOTUxODIzMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzk3MDJmODBhMTIwNTMzZGUiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3Iiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsInN0YXR1cyI6ImRpc21pc3NlZF9ieV9odW1hbiJ9LHsiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifV19 -->

Review

Findings

Medium

  • [logic-error] skills/pr-review/github/SKILL.md:163 — The new "Dependency-PR scoping" command, which the review agent runs in its own sandbox Bash tool, uses two constructs the sandbox command scanner (Tirith) rejects as unparseable: read -r -a DEP_PR_LIST <<< "${DEP_PRS:-}" (line 163) and the compound [[ $PID =~ ... && $EXACT =~ ... ]] in ids() (line 182). Each construct in isolation was blocked in this review sandbox with analysis_incomplete: Nested executable body could not be resolved, while a single [[ $d =~ ^[1-9][0-9]*$ ]] and for d in "${arr[@]}" passed. The same file documents the scanner dialect (line 63), and scripts/pr-review-dependency-scope-test.sh runs the snippet under plain bash with a stubbed gh, so it cannot catch this. As written the command cannot be executed verbatim, so the agent must improvise a rewrite or skip scoping.
    Remediation: Rewrite to the accepted dialect. Replace the herestring with an unquoted word-split loop (for d in $DEP_PRS; do) and validate each item with case "$d" in 0*|*[!0-9]*) SKIP=...; break ;; esac (glob arm first), or with printf '%s' "$d" | grep -Eqx '[1-9][0-9]*'. Replace the [[ ... && ... ]] check in ids() with two separate printf '%s' "$PID" | grep -Eqx '[0-9a-f]{40,64}' and printf '%s' "$EXACT" | grep -Eqx '[0-9a-f]{64}' checks. Re-verify each block through the sandbox Bash tool.
  • [protected-path] skills/pr-review/SKILL.md — This PR modifies protected paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md and skills/pr-review/references/dependency-scope.md. It links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.

Earlier findings

  • skills/pr-review/SKILL.md (docs), incorrect-doc — resolved by change: the cited region is no longer part of the PR's changes; the routing detail now lives in references/dependency-scope.md.
  • skills/pr-review/SKILL.md (docs), incomplete-doc — resolved by change: no changes remain near the cited region; challenger context scoping is specified in references/dependency-scope.md.
  • skills/pr-review/meta-prompt.md, incorrect-doc — resolved by change: the file is no longer modified by this PR.
  • skills/pr-review/SKILL.md (docs), incomplete-doc — resolved by change: no changes remain near the cited region; the review-body line is specified in references/dependency-scope.md.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (7)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImluY29ycmVjdC1kb2MiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9TS0lMTC5tZCIsImlkIjoiZl9lYzY1OTc4NDYwNjI2MWEzIiwibGluZSI6NzYzLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJpbmNvbXBsZXRlLWRvYyIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmXzgyMTA1YTM5YTQyNjc3OTAiLCJsaW5lIjoxMDAyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl9lYmUwNDc4NTk0NzJhMGM5IiwibGluZSI6MTQ5LCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZGVzaWduLXNtZWxsIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmX2Q3MjYzZjY5MDk1MTgyMzAiLCJsaW5lIjoxNDksImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJ1bmF1dGhvcml6ZWQtY2hhbmdlIiwiZmlsZSI6ImFnZW50cy9yZXZpZXcubWQiLCJpZCI6ImZfOTcwMmY4MGExMjA1MzNkZSIsImxpbmUiOjI1OSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl8xMDAxZDRiMjE2Y2MwNjg3IiwibGluZSI6MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImRlc2lnbi1kaXJlY3Rpb24iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfNmU1OTE2ZTc1MTlmNGMwMCIsImxpbmUiOjEsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvbWV0YS1wcm9tcHQubWQiLCJpZCI6ImZfOTcxNTI0NTAyOGI5MjBlNSIsImxpbmUiOjQsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmNvcnJlY3QtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmXzE3NGQ1YjU0MjAwZGM3MzciLCJsaW5lIjo5MiwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfN2I3MzUyMzdiOTkyYzIzNiIsImxpbmUiOjEzNTAsImFjdGlvbmFibGUiOmZhbHNlfV19 -->

Review

Findings

Medium

  • [protected-path] skills/pr-review/SKILL.md — This PR modifies protected paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, scripts/skill-env-reachability-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md. The PR links Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes.
  • [incorrect-doc] skills/pr-review/SKILL.md:763 — The step 4 Part 4 context-package template hardcodes pr-diff.txt (line 765) and has no Dependency scope section. The step 3d field list (608-620) also omits dependency_scope. Both conflict with line 611 (security uses pr-full-diff.txt after SCOPED) and with dependency-scope.md. A literal reading could give the security sub-agent only the scoped own-commit diff. See also the unchanged meta-prompt.md, which names pr-diff.txt.
    Remediation: Point the template Diff section at the assigned diff path (pr-full-diff.txt for security after SCOPED), add an optional ### Dependency scope section, and add dependency_scope to the step 3d list.
  • [incomplete-doc] skills/pr-review/SKILL.md:1002 — Step 6d says the challenger gets "the full PR diff", but its template hardcodes pr-diff.txt (the scoped diff under SCOPED) and omits dependency_scope. The challenger could remove security findings on dependency code as "not in the diff".
    Remediation: Clarify that pr-diff.txt is the own-commit diff on scoped PRs, reference pr-full-diff.txt, and add the Dependency scope section to the challenger template.
  • [scope-creep] skills/pr-review/github/SKILL.md:149 — Issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 asks for detect-and-filter. The PR adds an ~85-line bash procedure, a 120-line reference, a ~490-line test, a security re-route, a head-SHA pin, patch-id plus exact-hash matching, caps, and allowlist/Makefile edits. Against AGENTS.md rules 2 and 3 this is heavy for one reported pattern; the issue does request cherry-pick detection, so the excess is in the hardening layers. Advisory.
  • [design-smell] skills/pr-review/github/SKILL.md:149 — A large stateful bash program run by the agent that overwrites the shared pr-diff.txt in place and needs two env vars supplied on call. Siblings are short fetch snippets. A tested helper script would fit better. Advisory.

Low

  • [unauthorized-change] agents/review.md:259 — The added sentence about dependency scoping is a behavior-free disclaimer on the approval criteria, repeated in docs/review.md and the reference doc. The issue does not name it.
  • [missing-authorization] skills/pr-review/references/dependency-scope.md:1 — Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 is labelled feature and a maintainer asked for a relevance check before code work. Whether that human confirmation happened is not visible from the available data.
  • [design-direction] skills/pr-review/references/dependency-scope.md:1 — Only same-repo, author-declared, open dependencies with the "dependency commits first" shape are scoped. Reordered, squashed or conflict-resolved picks fall back to the full diff, so the original incident may not be covered.
  • [incorrect-doc] skills/pr-review/meta-prompt.md:4 — Unchanged by this PR; names pr-diff.txt as the untrusted diff and does not mention pr-full-diff.txt.
  • [incorrect-doc] skills/pr-review/references/dependency-scope.md:92 — "Synthesis (6e)" blurs step 6 (synthesis) with 6e (post-challenger checks).
  • [incomplete-doc] skills/pr-review/SKILL.md:1350 — Step 7 does not document the scoped-review attribution line that dependency-scope.md and docs/review.md require.

Labels: The PR adds a new capability to the review skill (dependency-scoped review).


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (8)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb21wbGV4aXR5LXJhdGlvIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJsaW5lIjoxNTMsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8zZGJlNDQ5YmVkNzE1NDNlIiwibGluZSI6MTgwLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImhpZ2giLCJjYXRlZ29yeSI6ImZhaWwtb3BlbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl9hYzY0NzM3YTMwYmRhYzZhIiwibGluZSI6NzB9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluamVjdGlvbi12dWxuIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzA4OGRjZjkzOTkwOGNjNzEiLCJsaW5lIjoxNjN9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzA4ZTVkM2U1Mzk4NmIxOTEiLCJsaW5lIjoyMTN9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InNjb3BlLWNyZWVwIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfZTVhM2QzZTU2ZmRmZWQ2NCIsImxpbmUiOjExN30seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiY29kZS1vcmdhbml6YXRpb24iLCJmaWxlIjoiZG9jcy9yZXZpZXcubWQiLCJpZCI6ImZfN2ZlMjc2MmRiZGQ0NzE1MiIsImxpbmUiOjE0MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6ImRvY3MvcmV2aWV3Lm1kIiwiaWQiOiJmXzU0OTBkMjdlNWVhOGI0NzYiLCJsaW5lIjoxNDYsImFjdGlvbmFibGUiOmZhbHNlfV0sImRpc3Bvc2l0aW9ucyI6W3siaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJzdGF0dXMiOiJkaXNtaXNzZWRfYnlfaHVtYW4ifSx7ImlkIjoiZl8zZGJlNDQ5YmVkNzE1NDNlIiwic3RhdHVzIjoiZGlzbWlzc2VkX2J5X2h1bWFuIn0seyJpZCI6ImZfYWM2NDczN2EzMGJkYWM2YSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzA4OGRjZjkzOTkwOGNjNzEiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl8wOGU1ZDNlNTM5ODZiMTkxIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfZTVhM2QzZTU2ZmRmZWQ2NCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzdmZTI3NjJkYmRkNDcxNTIiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl81NDkwZDI3ZTVlYThiNDc2Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn1dfQ== -->

Review

Findings

Medium

  • [protected-path] skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md, agents/review.md, scripts/pr-review-dependency-scope-test.sh, scripts/skill-env-reachability-test.sh — This PR modifies protected governance/infrastructure paths (skills/, agents/, scripts/). It links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes regardless of context.
    Remediation: A human maintainer with authority over these paths must review and approve.
Previous run (9)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb21wbGV4aXR5LXJhdGlvIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJsaW5lIjoxNTMsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8zZGJlNDQ5YmVkNzE1NDNlIiwibGluZSI6MTgwLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImhpZ2giLCJjYXRlZ29yeSI6ImZhaWwtb3BlbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L3JlZmVyZW5jZXMvZGVwZW5kZW5jeS1zY29wZS5tZCIsImlkIjoiZl9hYzY0NzM3YTMwYmRhYzZhIiwibGluZSI6NzB9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluamVjdGlvbi12dWxuIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzA4OGRjZjkzOTkwOGNjNzEiLCJsaW5lIjoxNjN9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzA4ZTVkM2U1Mzk4NmIxOTEiLCJsaW5lIjoyMTN9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InNjb3BlLWNyZWVwIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvU0tJTEwubWQiLCJpZCI6ImZfZTVhM2QzZTU2ZmRmZWQ2NCIsImxpbmUiOjExN30seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiY29kZS1vcmdhbml6YXRpb24iLCJmaWxlIjoiZG9jcy9yZXZpZXcubWQiLCJpZCI6ImZfN2ZlMjc2MmRiZGQ0NzE1MiIsImxpbmUiOjE0MSwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImluY29tcGxldGUtZG9jIiwiZmlsZSI6ImRvY3MvcmV2aWV3Lm1kIiwiaWQiOiJmXzU0OTBkMjdlNWVhOGI0NzYiLCJsaW5lIjoxNDYsImFjdGlvbmFibGUiOmZhbHNlfV0sImRpc3Bvc2l0aW9ucyI6W3siaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJzdGF0dXMiOiJvcGVuIn0seyJpZCI6ImZfYWM2NDczN2EzMGJkYWM2YSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzA4OGRjZjkzOTkwOGNjNzEiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl8wOGU1ZDNlNTM5ODZiMTkxIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfZTVhM2QzZTU2ZmRmZWQ2NCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzdmZTI3NjJkYmRkNDcxNTIiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl81NDkwZDI3ZTVlYThiNDc2Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn1dfQ== -->

Review

Findings

Medium

  • [protected-path] agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md — This PR modifies governance/infrastructure files under protected paths. The PR links Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, but human approval is always required for protected-path changes, regardless of context.

Low

  • [complexity-ratio] skills/pr-review/github/SKILL.md:153 — The "Dependency-PR scoping" bash command is still about 90 lines (153–239). It does patch-id and exact-hash commit matching, shape validation, a pinned-head race check, compare-API safety checks and a diff swap, which goes well beyond the minimum issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 asks for. The block is unchanged since the prior review.
    Remediation: Optional: cut the block to the minimum the issue needs, or move the logic into a tested script that the skill calls.
  • [logic-error] skills/pr-review/github/SKILL.md:180 — The whitespace-exact hash (grep -E '^[-+]' "$W/one.diff" | sha256sum) drops Git's \ No newline at end of file markers, and git patch-id --stable ignores them too. Two commits with the same added/deleted text but different final-newline handling therefore get identical match IDs, so a cherry-pick differing only in that way is treated as a dependency commit and left out of the scoped non-security diff. Security review still reads the full diff.
    Remediation: Include the \ No newline at end of file marker lines in the exact fingerprint, and add a contract test showing that a cherry-pick differing only in final-newline handling stays in scope.
Previous run (10)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb21wbGV4aXR5LXJhdGlvIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJsaW5lIjoxNTgsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoiaGlnaCIsImNhdGVnb3J5IjoiZmFpbC1vcGVuIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvcmVmZXJlbmNlcy9kZXBlbmRlbmN5LXNjb3BlLm1kIiwiaWQiOiJmX2FjNjQ3MzdhMzBiZGFjNmEiLCJsaW5lIjo3MH0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiaW5qZWN0aW9uLXZ1bG4iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfMDg4ZGNmOTM5OTA4Y2M3MSIsImxpbmUiOjE2M30seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibG9naWMtZXJyb3IiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfMDhlNWQzZTUzOTg2YjE5MSIsImxpbmUiOjIxM30seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5Ijoic2NvcGUtY3JlZXAiLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9TS0lMTC5tZCIsImlkIjoiZl9lNWEzZDNlNTZmZGZlZDY0IiwibGluZSI6MTE3fSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb2RlLW9yZ2FuaXphdGlvbiIsImZpbGUiOiJkb2NzL3Jldmlldy5tZCIsImlkIjoiZl83ZmUyNzYyZGJkZDQ3MTUyIiwibGluZSI6MTQxLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiaW5jb21wbGV0ZS1kb2MiLCJmaWxlIjoiZG9jcy9yZXZpZXcubWQiLCJpZCI6ImZfNTQ5MGQyN2U1ZWE4YjQ3NiIsImxpbmUiOjE0NiwiYWN0aW9uYWJsZSI6ZmFsc2V9XSwiZGlzcG9zaXRpb25zIjpbeyJpZCI6ImZfNmI3NWU4ZWVjNjE2YzJkZiIsInN0YXR1cyI6Im9wZW4ifSx7ImlkIjoiZl83ZmUyNzYyZGJkZDQ3MTUyIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfNTQ5MGQyN2U1ZWE4YjQ3NiIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmX2FjNjQ3MzdhMzBiZGFjNmEiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl8wODhkY2Y5Mzk5MDhjYzcxIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfMDhlNWQzZTUzOTg2YjE5MSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmX2U1YTNkM2U1NmZkZmVkNjQiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifV19 -->

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, skills/pr-review/references/dependency-scope.md. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 (Closes #1654) and explains the rationale, but human approval is always required for protected-path changes, regardless of context.

Low

  • [complexity-ratio] skills/pr-review/github/SKILL.md:158 — The Dependency-PR scoping snippet is about 80 lines of dense bash (patch-id plus whitespace-exact hashing, head pinning against PINNED_SHA, compare validation, safe_path sanitisation), backed by a roughly 466-line stub-gh test harness. Issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654's "at minimum" ask was smaller. Most of the extra machinery is fail-closed defence, but the ratio of mechanism to need is still high. Unchanged since the prior review, so the prior severity is kept.
    Remediation: Consider simplifying the two-hash cherry-pick matching, or moving the snippet into a standalone script.

Earlier findings

  • f_7fe2762dbdd47152 — resolved_by_change: the "Stacked and cherry-picked PRs" section in docs/review.md moved out from between the intro sentence and the numbered pipeline list to after the re-review paragraph (see the diff for docs/review.md).
  • f_5490d27e5ea8b476 — resolved_by_change: the same section now states that security inspection, protected-path checks and PR-body defenses cover the whole PR, and that GitLab and unmatched commits get the full diff (see the diff for docs/review.md).
Previous run (11)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJjb21tZW50IiwiZmluZGluZ3MiOlt7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb21wbGV4aXR5LXJhdGlvIiwiZmlsZSI6InNraWxscy9wci1yZXZpZXcvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJsaW5lIjoxNTgsImFjdGlvbmFibGUiOmZhbHNlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJjb2RlLW9yZ2FuaXphdGlvbiIsImZpbGUiOiJkb2NzL3Jldmlldy5tZCIsImlkIjoiZl83ZmUyNzYyZGJkZDQ3MTUyIiwibGluZSI6MTQxLCJhY3Rpb25hYmxlIjpmYWxzZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiaW5jb21wbGV0ZS1kb2MiLCJmaWxlIjoiZG9jcy9yZXZpZXcubWQiLCJpZCI6ImZfNTQ5MGQyN2U1ZWE4YjQ3NiIsImxpbmUiOjE0NiwiYWN0aW9uYWJsZSI6ZmFsc2V9LHsic2V2ZXJpdHkiOiJoaWdoIiwiY2F0ZWdvcnkiOiJmYWlsLW9wZW4iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfYWM2NDczN2EzMGJkYWM2YSIsImxpbmUiOjcwfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmplY3Rpb24tdnVsbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8wODhkY2Y5Mzk5MDhjYzcxIiwibGluZSI6MTYzfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8wOGU1ZDNlNTM5ODZiMTkxIiwibGluZSI6MjEzfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmX2U1YTNkM2U1NmZkZmVkNjQiLCJsaW5lIjoxMTd9XSwiZGlzcG9zaXRpb25zIjpbeyJpZCI6ImZfYWM2NDczN2EzMGJkYWM2YSIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzA4OGRjZjkzOTkwOGNjNzEiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifSx7ImlkIjoiZl8wOGU1ZDNlNTM5ODZiMTkxIiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfZTVhM2QzZTU2ZmRmZWQ2NCIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzZiNzVlOGVlYzYxNmMyZGYiLCJzdGF0dXMiOiJvcGVuIn0seyJpZCI6ImZfN2ZlMjc2MmRiZGQ0NzE1MiIsInN0YXR1cyI6Im9wZW4ifV19 -->

Review

Findings

Medium

Low

  • [complexity-ratio] skills/pr-review/github/SKILL.md:158 — The dependency-scoping detection is still a large inline bash snippet (~lines 154-228), and this push enlarges it (second compare call, DEP_PRS array parsing, numeric validation). The hardening directly fixes prior findings, and the contract test exercises the snippet, so the remaining concern is size and reviewability.
    Remediation: Consider moving the logic into a versioned script that the existing test harness calls directly, or trimming it to the minimum needed.
  • [code-organization] docs/review.md:141 — The ### Stacked and cherry-picked PRs subsection now sits between the pipeline intro sentence under ## How the agent works and its numbered steps, so the steps read as nested under it.
    Remediation: Move the subsection below the numbered pipeline steps and the re-review paragraph (before ## Time budget).
  • [incomplete-doc] docs/review.md:146 — The new section says the review covers only the PR's own commits and that protected-path and PR-body checks cover the whole PR. Per references/dependency-scope.md and pr-review/SKILL.md, security inspection also covers the whole PR, and scoping is GitHub-only (GitLab reviews the full diff). The doc omits both.
    Remediation: State that non-security dimensions review only own commits, that security inspection covers the whole PR, and that scoping is GitHub-only.

Earlier findings

  • f_ac64737a30bdac6a — resolved_by_change: security dispatch and inspection now use the full PR diff and file list (dependency-scope.md "Security dispatch" bullet; SKILL.md step 3d).
  • f_088dcf939908cc71 — resolved_by_change: DEP_PRS parsed with read -r -a and validated as a positive integer (github/SKILL.md; new malformed-candidate test).
  • f_08e5d3e53986b191 — resolved_by_change: scoped diff now comes from the raw compare diff, preserving mode metadata (github/SKILL.md; new content-and-mode test).
  • f_e5a3d3e56fdfed64 — resolved_by_change: step 1 wording at PR head matches the base branch; the PR no longer touches it.
Previous run (12)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJoaWdoIiwiY2F0ZWdvcnkiOiJmYWlsLW9wZW4iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9yZWZlcmVuY2VzL2RlcGVuZGVuY3ktc2NvcGUubWQiLCJpZCI6ImZfYWM2NDczN2EzMGJkYWM2YSIsImxpbmUiOjcwfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJpbmplY3Rpb24tdnVsbiIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8wODhkY2Y5Mzk5MDhjYzcxIiwibGluZSI6MTYzfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8wOGU1ZDNlNTM5ODZiMTkxIiwibGluZSI6MjEzfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJzY29wZS1jcmVlcCIsImZpbGUiOiJza2lsbHMvcHItcmV2aWV3L1NLSUxMLm1kIiwiaWQiOiJmX2U1YTNkM2U1NmZkZmVkNjQiLCJsaW5lIjoxMTd9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImNvbXBsZXhpdHktcmF0aW8iLCJmaWxlIjoic2tpbGxzL3ByLXJldmlldy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfNmI3NWU4ZWVjNjE2YzJkZiIsImxpbmUiOjE0OX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiY29kZS1vcmdhbml6YXRpb24iLCJmaWxlIjoiZG9jcy9yZXZpZXcubWQiLCJpZCI6ImZfN2ZlMjc2MmRiZGQ0NzE1MiIsImxpbmUiOjg0fV19 -->

Review

Findings

High

  • [fail-open] skills/pr-review/references/dependency-scope.md:70 — Scoping replaces the review diff and changed_files before triage, but nothing requires the security dispatch decision to use the original full PR. Step 3b of skills/pr-review/SKILL.md classifies domains from the diff and changed-file list, and step 3c dispatches security only on that classification. Once SCOPED, the reference makes pr-diff.txt the own-commit diff and sets changed_files from dep-scope/files. An author can put a security-relevant change in an open dependency PR, duplicate that commit here, and add a harmless own commit; the scoped inputs may then not trigger security. The "inspect the whole PR" note and the rule against dropping security findings only apply to agents that are actually dispatched. Merging this PR merges the dependency commits too.
    Remediation: Decide security dispatch and its exploration budget from the original full diff and full changed-file list. When security is selected, give it pr-full-diff.txt as its primary diff and the full changed-file list. Add a dispatch test where a security-relevant dependency commit is followed by a documentation-only own commit.

Medium

  • [protected-path] skills/pr-review/SKILL.md — This PR modifies protected paths: agents/review.md, scripts/pr-review-dependency-scope-test.sh, skills/pr-review/SKILL.md, skills/pr-review/github/SKILL.md, and skills/pr-review/references/dependency-scope.md. The PR links issue Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 and explains the rationale, so context is sufficient, but human approval is always required for protected-path changes regardless of context.

Low

  • [injection-vuln] skills/pr-review/github/SKILL.md:163 — DEP_PRS comes from untrusted author text, but the snippet does not check that each candidate is a positive decimal PR number, and it does not disable pathname expansion in for d in $DEP_PRS. Each d goes into API paths (repos/.../pulls/$d) and a workspace filename ($W/dep-$d.json). Quoting later uses stops shell-command evaluation but not embedded path or query syntax, and the unquoted loop also expands wildcards.
    Remediation: Parse the candidate list without pathname expansion, and require every candidate to match ^[1-9][0-9]*$ before making requests or building filenames. On malformed input, leave the full diff unchanged and report SKIP. Add tests for malformed and wildcard candidates.
  • [logic-error] skills/pr-review/github/SKILL.md:213 — The scoped diff is rebuilt from the compare API's JSON .patch hunks plus a synthetic diff --git header, which drops file-mode metadata. If an own commit changes a file's content and also flips its executable bit, pr-diff.txt shows the content change but not the mode lines. A mode-only change has no .patch and falls back to the full diff, so only combined content-and-mode commits are affected. The tests cover only text patches.
    Remediation: Fetch LAST_DEP...PINNED_SHA with the application/vnd.github.diff media type, or add the mode change to the synthetic header. Keep the completeness checks and full-diff fallback. Add a test for a commit that changes both content and mode.
  • [scope-creep] skills/pr-review/SKILL.md:117 — Two edits in step 1 are unrelated to stacked-PR scoping: the bullet "If none was provided, stop..." was reworded to "Otherwise stop...", and the paragraph "If no PR can be identified, stop and report the failure..." was deleted. Both are cosmetic cleanups of lines the issue did not authorize. They do not change behavior.
    Remediation: Revert these two hunks, or move them to a separate cleanup PR.
  • [complexity-ratio] skills/pr-review/github/SKILL.md:149 — The issue asks, at minimum, to check whether the author declared a dependency and to cross-reference commits. The PR adds a roughly 75-line bash snippet to the forge skill, a 119-line reference doc, and a 411-line test script that extracts code from the markdown. The design is conservative, since it falls back to the full diff, but it is heavy and costly to maintain.
    Remediation: No blocking change. A maintainer should confirm they accept this complexity, or the logic could move into a versioned script under scripts/.
  • [code-organization] docs/review.md:84 — The subsection ### Stacked and cherry-picked PRs sits under ## Control labels, before ## Configuration. It describes how the agent scopes diffs, not a control label. Sibling agent docs keep behavior subsections under ## How the agent works.
    Remediation: Move the subsection under ## How the agent works, or make it an H2 section.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 7:35 AM UTC · Completed 7:40 AM UTC

Commit: 1f1ffe7 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.66

fullsend-ai-coder Bot added a commit that referenced this pull request Oct 12, 2026
Require a whitespace-exact match (in addition to the stable patch-id)
before treating a cherry-picked commit as a dependency commit, and
refuse to scope when any commit list or commit diff cannot be fetched.
Guard the dependency loop and SKIP reasons, read all comment pages for
dependency declarations, document scoped-file and re-review behavior,
restore the PR-identification stop sentence, and document the
open-dependency approval hold in docs/review.md and agents/review.md.

Not run: pre-commit (hook repositories unreachable from the sandbox) and
skillsaw (unavailable); lint-agent-docs, gitleaks and the pr-review
script tests pass.

Addresses #1661

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed all findings. The main change makes cherry-pick matching whitespace-exact and fails closed when commit diffs cannot be fetched, with new tests. Also fixed the loop and SKIP handling, pagination wording, and docs, and restored the step 1 sentence. The protected-path finding needs human approval and prescribes no edit. Pre-commit could not fetch hooks and skillsaw was unavailable; gitleaks, lint-agent-docs and the pr-review script tests passed.

Fixed (9):

  1. Stable patch-id ignores whitespace, so a conflict-resolved cherry-pick differing only in indentation is classified as dependency code (logic-error and security-review-bypass, same root cause) (skills/pr-review/github/SKILL.md): Patch IDs now only select candidates; a commit is excluded only if a whitespace-exact hash of its changed lines also matches the dependency commit. Added whitespace-only regression test and documented the rule in dependency-scope.md.
  2. Failed commit-diff retrieval is treated as no match instead of setting SKIP (skills/pr-review/github/SKILL.md): Retrieval and patch-ID generation are now checked via a helper; any failure sets SKIP (dependency or own commits) so scoping is refused. The final shape check no longer overwrites an earlier SKIP reason. Added two tests for unavailable diffs.
  3. dep-scope/files can list paths absent from the step 2b manifest (skills/pr-review/references/dependency-scope.md): Documented that such paths equal base at HEAD_SHA and are read from target-repo/.
  4. Author comments command has no pagination, so later dependency declarations are missed (skills/pr-review/references/dependency-scope.md): Step 1 now requires --paginate --slurp so every page of comments is read before filtering by the author; a test asserts the wording.
  5. Dependency loop runs when SKIP is set and one assignment overwrites the root-cause reason (skills/pr-review/github/SKILL.md): Wrapped the loop in a test -z SKIP guard and used the SKIP default-preserving form on the list-commits assignment.
  6. Deleted step 1 stop sentence does not trace to Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs #1654 (skills/pr-review/SKILL.md): Restored the stop-and-report behavior by extending the remaining bullet to cover a PR that cannot be identified, at a net cost of a few tokens. skillsaw is unavailable in the sandbox, so the context-budget ceiling was not re-measured.
  7. Interaction between scoped pr-diff.txt and step 2a changed_since_prior is undocumented (skills/pr-review/SKILL.md): Added a note in dependency-scope.md (kept out of SKILL.md for budget): the prior-review compare spans all commits so changed_since_prior may list dependency paths, its all fallback uses the scoped diff, and findings follow the Synthesis rule.
  8. docs/review.md does not describe dependency scoping or the approval hold (docs/review.md): Added a Stacked and cherry-picked PRs subsection stating own-commit scoping, the comment verdict with requires-manual-review while a dependency PR is open, and full-diff fallback.
  9. agents/review.md Outcome list omits the open-dependency-PR approval condition (agents/review.md): Added the open-dependency-PR condition to the approve bullet.

Disagreed (1):

  1. protected-path: PR modifies protected paths: This finding only demands human approval under the merge gate and prescribes no content change; the PR needs a human approver.

Tests: passed

CI inspections (6):

  1. test (passing, success)
  2. skillsaw (passing, success): Passed on the reviewed commit; not re-run after this fix because skillsaw is unavailable in the sandbox.
  3. script-test (passing, success)
  4. commit-lint (passing, success)
  5. DCO (passing, success)
  6. functional-tests-complete (transient-infra, failure): The rollup gate fails closed on label events because an earlier run on the same SHA was cancelled or failed; no functional tests ran (all skipped). It does not reflect the PR diff. Pushing the new commit triggers a fresh run; rerun the job if it persists.

Conflict update: skipped; strategy none; forge MERGEABLE; target main@d8a9bc78b9566158a03efda6265d904c7dfc1981

Decision points
  • How to restore the deleted step 1 sentence without raising the skillsaw token ceiling (alternatives: Restore the paragraph verbatim, Fold it into the existing bullet, Trim unrelated text; rationale: Folding it into the bullet restores the behavior with minimal added tokens and avoids unrelated edits.)
  • Where to put the step 2a interaction note (alternatives: SKILL.md, dependency-scope.md; rationale: SKILL.md is at a baselined token ceiling; the reference file is the natural home for scoping details.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:41 AM UTC · Completed 7:53 AM UTC

Commit: 87b35d5 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.91

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 7:55 AM UTC · Completed 7:59 AM UTC

Commit: 87b35d5 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.54

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed the medium ledger finding and all five low findings; the protected-path notice is recorded as a disagreement because it is a merge gate with no content edit. The dependency-scope script tests pass, including new cases for head-SHA resolution and a failed hash step. The skillsaw CI job fails because skills/pr-review/SKILL.md is a few tokens over its baseline ceiling; that needs a protected-path edit or a baseline change, so a human should decide. The skillsaw linter could not run locally.

Fixed (6):

  1. Dropping dependency findings is not reconciled with the prior-finding ledger (skills/pr-review/references/dependency-scope.md): Synthesis rule now exempts open prior finding ids from the drop. A prior id whose code is newly excluded gets a reclassified disposition (rationale names the dependency PR) and an info-severity row carrying the same id, so the ledger closes it out instead of carrying it open. Added wiring assertions to the test script.
  2. HEAD_SHA used in the scoping snippet without being set in the same call (skills/pr-review/github/SKILL.md): The snippet now re-derives the head SHA from the pull request API before the compare and skips scoping if it cannot be determined. The test no longer injects HEAD_SHA, and a new case covers an unresolvable head SHA.
  3. Prompt-injection findings in excluded dependency commits are suppressed (skills/pr-review/references/dependency-scope.md): The drop rule and the context-package note now exempt prompt-injection findings. Injection inspection of source content still covers the excluded commits via the full diff.
  4. Missing pipefail lets a failed hash step produce an empty value that matches (skills/pr-review/github/SKILL.md): ids() now requires a well-formed patch-id and a 64-hex whitespace-exact hash, otherwise it fails and scoping is skipped. A new test stubs a failing hash tool and checks the fallback.
  5. Approve-withholding rule is prompt-level only but docs say the review never approves (docs/review.md): Softened the docs to say the agent is instructed not to approve while the dependency PR is open, noted that it is a prompt-level instruction rather than a post-script gate, and limited it to the scoped case. Maintainer sign-off on the policy itself remains a human decision.
  6. Stacked-PR docs section sits under the wrong heading context (docs/review.md): Moved the issue-labels paragraph back under its original heading, placed the new section after it, and stated the approve restriction applies only when the diff was scoped.

Disagreed (1):

  1. Protected-path changes require human approval: This is the mandatory merge-gate notice. It asks for human approval and prescribes no content edit, so there is nothing to change in the code.

Tests: passed

CI inspections (6):

  1. skillsaw (pr-related, failure): The context-budget rule reports skills/pr-review/SKILL.md a few tokens above its baseline ceiling, because this PR added lines to that file. Not fixed. Trim a few tokens from skills/pr-review/SKILL.md (protected path, needs human direction) or regenerate the baseline with maintainer approval.
  2. script-test (passing, success): Script tests passed.
  3. test (passing, success): Unit tests passed.
  4. commit-lint (passing, success): Commit lint passed.
  5. behaviour (passing, success): Behaviour tests passed.
  6. DCO (passing, success): DCO check passed.

Conflict update: skipped; strategy none; forge MERGEABLE; target main

Decision points
  • How to treat a prior finding whose code is newly excluded by dependency scoping (alternatives: Drop it silently, Mark it resolved_by_change, Reclassify to info with the same id; rationale: Reclassifying matches the existing ledger mechanism and avoids both an indefinitely open id and a false resolution claim.)
  • Leave the skillsaw context-budget CI failure unfixed (alternatives: Trim skills/pr-review/SKILL.md, Raise the baseline ceiling; rationale: The fix needs an edit to a protected path that no finding authorizes, and raising the ceiling goes against the earlier effort to stay within the baseline.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Oct 12, 2026
Reconcile dependency scoping with the prior-finding ledger, keep
prompt-injection findings out of the drop rule, re-derive HEAD_SHA in the
scoping snippet, validate patch-id/hash output, and tighten the docs.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:01 AM UTC · Completed 8:14 AM UTC

Commit: 1885c5a · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.71

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 8:16 AM UTC · Completed 8:21 AM UTC

Commit: 1885c5a · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.74

fullsend-ai-coder Bot added a commit that referenced this pull request Oct 12, 2026
- pin dependency scoping to the step 1 head SHA (PINNED_SHA) and skip
  when the live PR head differs; compare against the pinned SHA
- validate compare filename/previous_filename with safe_path and skip
  on unsafe paths
- fetch scoped paths missing from the manifest at HEAD_SHA instead of
  reading the base checkout
- protect all agent-targeting injection categories (instruction-smuggling,
  unicode-steganography, bidi-override, homoglyph-attack) from the
  dependency drop rules
- trim the SKILL.md step 1 wording back under the skillsaw context-budget
  baseline ceiling
- add tests for moved head, missing pin, and newline filenames

Disagreements: scope-creep finding on agents/review.md (policy decision
for maintainers) and the protected-path merge-gate finding are not
code changes.

Addresses #1661

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (bot-triggered)

Addressed the third review round and the pre-commit shellcheck SC2209 failure by quoting the PINNED assignment. Two findings were recorded as disagreements. The dependency-scope tests pass; pre-commit hook repos are unreachable from the sandbox, so the shellcheck re-run was not possible locally.

Fixed (6):

  1. Dependency scoping must be pinned to the step 1 head SHA (skills/pr-review/github/SKILL.md): Pin scoping to PINNED_SHA, skip when the live PR head differs, and compare against the pinned SHA; add tests for moved head and missing pin.
  2. Compare filenames and previous_filename are not validated (skills/pr-review/github/SKILL.md): Validate with safe_path and skip scoping on unsafe paths; add newline-filename tests.
  3. Scoped paths missing from the manifest are read from the base checkout (skills/pr-review/github/SKILL.md): Fetch such paths at HEAD_SHA instead.
  4. Agent-targeting injection categories must be protected from dependency drop rules (skills/pr-review/references/dependency-scope.md): Protect instruction-smuggling, unicode-steganography, bidi-override and homoglyph-attack findings from the drop rules.
  5. skillsaw context-budget ceiling exceeded by SKILL.md step 1 wording (skills/pr-review/SKILL.md): Trimmed the step 1 wording back under the baseline ceiling.
  6. Pre-commit shellcheck SC2209 on PINNED=head in the test script (scripts/pr-review-dependency-scope-test.sh): Quoted the assignment as PINNED="head"; the dependency-scope tests still pass.

Disagreed (2):

  1. Scope creep on agents/review.md: Policy decision for maintainers, not a code change.
  2. Protected-path merge-gate finding: Mandatory merge-gate finding that only requires human approval; no content edit is prescribed.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:23 AM UTC · Completed 8:30 AM UTC

Commit: 22df29f · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.35

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself October 12, 2026 08:29

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:43 PM UTC · Completed 1:49 PM UTC

Commit: b87d5bb · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.91

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself October 21, 2026 13:49

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Oct 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:35 PM UTC · Completed 3:42 PM UTC

Commit: 4fd6a00 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.07

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:05 PM UTC

Commit: 1afc750 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.48

@fullsend-ai-review fullsend-ai-review Bot added the enhancement New feature or request label Oct 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:16 PM UTC · Completed 1:39 PM UTC

Commit: 6cf0fcf · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.78

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Oct 23, 2026
@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1: Fix iteration 6 exceeds bot cap of 5. Escalating to human. The review→fix loop has run 6 times without converging. A human can still direct the agent with /fs-fix (u…) · Started 1:41 PM UTC · Completed 1:41 PM UTC

Commit: 6cf0fcf · View workflow run →

Effort: high

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:55 PM UTC · Completed 2:03 PM UTC

Commit: ad5358a · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.72

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1: Fix iteration 6 exceeds bot cap of 5. Escalating to human. The review→fix loop has run 6 times without converging. A human can still direct the agent with /fs-fix (u…) · Started 2:05 PM UTC · Completed 2:05 PM UTC

Commit: ad5358a · View workflow run →

Effort: high

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:31 PM UTC · Completed 5:53 PM UTC

Commit: 5250a73 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.05

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

ids() {
gh api -H "$DIFF_ACCEPT" "repos/${REPO_FULL_NAME}/commits/$1" < /dev/null > "$W/one.diff" || return 1
PID=$(git patch-id --stable < "$W/one.diff" | cut -d' ' -f1) || return 1
EXACT=$(grep -E '^([-+]|\\ No newline at end of file)' "$W/one.diff" | sha256sum | cut -d' ' -f1) || return 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] logic-error

An empty commit disables dependency scoping for an otherwise valid stack. git patch-id --stable produces no output for an empty diff, so ids() fails hash validation and sets SKIP. This also affects dependencies matched by exact SHA, because every dependency commit is hashed before SHA-based classification. A harmless CI-retrigger commit can therefore cause dependency code to receive findings again. The failure is fail-safe (the full diff is reviewed).

Suggested fix: Handle successfully fetched empty diffs separately from fetch or hashing failures, and avoid requiring patch IDs for dependency commits already matched by SHA. Add a regression case containing an empty commit.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure (running pre-script: exit status 1: Fix iteration 6 exceeds bot cap of 5. Escalating to human. The review→fix loop has run 6 times without converging. A human can still direct the agent with /fs-fix (u…) · Started 5:54 PM UTC · Completed 5:54 PM UTC

Commit: 5250a73 · View workflow run →

Effort: high

This branch was previously deployed

1 inactive deployment
dev — 5250a731 Deployed Oct 24, 2026 by shairevivo via behaviour #1253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs-human Agent loop needs human intervention ok-to-test ready-for-review Triggers review agent dispatch risk/elevated PR risk: elevated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review agent should scope diff analysis to incremental changes on stacked/cherry-picked PRs

1 participant