Skip to content

feat(#252): detect and flag PRs modifying deprecated scaffold paths #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 2 commits into from
Closed

feat(#252): detect and flag PRs modifying deprecated scaffold paths #276

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0d83db
feat(#252): detect and flag PRs modifying deprecated scaffold paths
fullsend-ai-coder[bot] Jul 10, 2026
977f993
fix(#252): address review feedback on PR #276
fullsend-ai-coder[bot] Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 58 additions & 8 deletions internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,12 +627,59 @@ attention.
If no protected files are modified, do not add a `protected-path`
finding.

##### Deprecated paths

Check whether the PR modifies files under deprecated paths. These are
locations whose canonical content has migrated to another repository.
Changes targeting them are almost certainly misdirected and will not
be merged.

@fullsend-ai-review fullsend-ai-review Bot Jul 10, 2026

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-ambiguity

Step 6f parenthetical exception for deprecated-path is slightly ambiguous when both a deprecated-path finding and separate critical/high findings coexist. The outcome resolves correctly via step 6e MUST-reject, but the 6f wording could be tightened to make the priority explicit.

Suggested fix: Rephrase the first bullet of 6f to state: if any deprecated-path finding is present, the outcome is reject regardless of other findings.


Deprecated path prefixes:

- `internal/scaffold/fullsend-repo/` — scaffold content has migrated
to the `fullsend-ai/agents` repository. PRs modifying files here
should target that repository instead.

**Exemptions:** PRs that *only* modify scaffold propagation or
versioning machinery are not misdirected. Exempt files are those
that exist solely to support the sync/deploy pipeline — for example,
`templates/shim-per-repo.yaml` and `templates/shim-workflow-call.yaml`
(workflow dispatch shims consumed by the propagation tooling). Agent
definitions, skills, schemas, scripts, policies, harness configs,
workflows, and environment files are never exempt. To qualify for the

@fullsend-ai-review fullsend-ai-review Bot Jul 10, 2026

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] exemption-underspecification

The exemption clause gives two example files but relies on agent judgment for other files. The two named files are the only files in templates/ today, making examples effectively exhaustive, but a clearer pattern would reduce ambiguity if the directory grows.

Suggested fix: Consider using a directory-based pattern (e.g., files under templates/) instead of individual file examples.

exemption, every file matched by the deprecated prefix must be an
exempt file. If any non-exempt file is present, the exemption does
not apply.

For each file in the PR diff, check whether its path starts with any
deprecated path prefix.

If **any** deprecated-path files are modified (and the exemption does
not apply):

Raise a **high** finding with category `deprecated-path`. The
description MUST list the affected deprecated-path files, state that
the content at this location has migrated to `fullsend-ai/agents`,
and direct the author to open the PR in the correct repository. Set
remediation to: "Close this PR and re-submit the changes in the
`fullsend-ai/agents` repository, which is the canonical location
for this content."

When a `deprecated-path` finding is raised, the outcome MUST NOT be
`approve` or `request-changes` — the outcome MUST be `reject`. The PR
is targeting the wrong location and no amount of code-level iteration
will make it mergeable.

If no deprecated-path files are modified, or if the exemption applies,
do not add a `deprecated-path` finding.

#### 6f. Determine overall outcome

Merge PR-specific findings into the challenger-adjudicated finding set
and evaluate:

- Any **critical** or **high** finding → `request-changes`
- Any **critical** or **high** finding → `request-changes` (except
`deprecated-path` findings, which force `reject` — see step 6e)
- Multiple **medium** findings which could affect the intended outcome
of the PR → `request-changes`
- One **medium** finding (but no critical/high) → `comment-only`
Expand All @@ -643,9 +690,9 @@ and evaluate:
`actionable: true` so the post-script can create follow-up issues)
- No findings → `approve`
- The approach is fundamentally wrong — wrong design, unauthorized
change, or the PR should be closed/completely rethought → `reject`.
Use `reject` only when no amount of code-level iteration will make
the PR mergeable.
change, deprecated-path finding, or the PR should be
closed/completely rethought → `reject`. Use `reject` only when no
amount of code-level iteration will make the PR mergeable.

### 7. Produce the review result

Expand Down Expand Up @@ -779,13 +826,16 @@ wins.

- **Never approve with unresolved critical or high findings.** If any
critical or high finding exists, the outcome must be
`request-changes`.
`request-changes` (unless a `deprecated-path` finding is present,
which forces `reject`).
- **Never approve when any protected-path finding exists**, regardless of
severity.
- **Never approve or request-changes when a deprecated-path finding
exists** — the outcome must be `reject`.
- **PR-specific checks (step 6e) belong in the orchestrator only.** Do
not push protected-path checks, scope authorization, or PR body
injection defense into sub-agents. These require PR-level context
that sub-agents do not have.
not push protected-path checks, deprecated-path checks, scope
authorization, or PR body injection defense into sub-agents. These
require PR-level context that sub-agents do not have.
- **All sub-agents must be dispatched simultaneously.** Include all
Agent calls in a single message. Sequential dispatch defeats the
architecture's purpose.
Expand Down
Loading