feat(#5650): add /fs-stop to skip auto agent triggers - #5920
Conversation
|
🤖 Finished Review · ✅ Success · Started 2:06 AM UTC · Completed 2:22 AM UTC |
PR Summary by QodoAdd /fs-stop to apply fullsend-no-* labels and gate auto agent dispatch
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Site previewPreview: https://5a592edd-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Labels: PR implements a new /fs-stop feature affecting dispatch routing, CI workflows, and documentation. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep — two additional findings not covered by existing comments on this PR.
7423e1f to
44e5cbb
Compare
|
Addressed the review-agent findings on head
|
|
🤖 Finished Review · ✅ Success · Started 4:10 PM UTC · Completed 4:32 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep — additional findings not covered by existing comments on this PR.
HIGH: reconcile-repos.sh drift detection never compares stop-agent.sh content, so future script fixes won't propagate to enrolled repos (internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh, around the enroll/update comparison loop — not attachable inline since that region falls outside this PR's diff hunks)
The enroll/update loop fetches only .github/workflows/fullsend.yaml (SHIM_PATH) from the remote repo, computes REMOTE_MANAGED/EXPECTED_MANAGED via managed_content_b64, and if they match prints "already enrolled (shim up to date)" and skips. STOP_AGENT_SCRIPT/STOP_AGENT_PATH (defined near the top of the file) are used only for a local-file existence check and to build the git tree in write_shim_to_branch_from_default when a write actually happens — there is no gh api repos/$ORG/$REPO/contents/$STOP_AGENT_PATH fetch or comparison anywhere in the file.
Once every enrolled repo's shim YAML converges to this PR's content (so the YAML comparison passes), this script will report every repo as "up to date" forever, even if .github/scripts/stop-agent.sh is edited later (e.g. a security fix analogous to issue #5421, which is precisely the class of authorization bug this script's logic guards against). Enrolled repos would silently keep running the stale/vulnerable script with no update PR ever generated.
Suggestion: fetch the deployed .github/scripts/stop-agent.sh via gh api repos/$ORG/$REPO/contents/$STOP_AGENT_PATH and include its content in the drift comparison (same "managed content vs expected content" pattern already used for the shim YAML), so a stop-agent.sh-only change is also detected as drift requiring an update PR.
Remaining findings are posted as inline comments on the diff.
44e5cbb to
3ea4ea5
Compare
|
@waynesun09 addressed your second review sweep on head Inline
Review-body HIGH (reconcile drift)
|
|
🤖 Finished Review · ✅ Success · Started 11:17 AM UTC · Completed 11:37 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional finding not anchorable to a diff line (outside the changed hunks in this PR):
MEDIUM: Unenrollment (Phase 2) never deletes stop-agent.sh, leaving it orphaned in disconnected repos — internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh, around line 594
This PR's write_shim_to_branch_from_default() now writes both SHIM_PATH (.github/workflows/fullsend.yaml) and the new STOP_AGENT_PATH (.github/scripts/stop-agent.sh) in one tree commit on enroll/update, and drift detection now also compares stop-agent.sh content. However, the unenroll/disable block only checks for and DELETEs SHIM_PATH — STOP_AGENT_PATH is never referenced anywhere in that block (confirmed the unenroll section is untouched by this PR's diff). After a repo is disabled in config.yaml, it keeps a dangling .github/scripts/stop-agent.sh even though the PR's own unenroll PR body text says the repo "will no longer dispatch events to the fullsend agent pipeline." No test in reconcile-repos-test.sh covers removal of stop-agent.sh on unenroll either.
Suggestion: Extend the unenroll block to also fetch the SHA of and DELETE .github/scripts/stop-agent.sh (mirroring the SHIM_PATH delete, or via a shared delete-file helper), guarding for repos enrolled before this PR merged (script may not exist yet on their branch). Add a reconcile-repos-test.sh case asserting the script is removed alongside the shim on unenroll.
3ea4ea5 to
4c0ad13
Compare
Extract stop-agent bash into a shared script, skip dispatch for stop comments, tolerate leading whitespace, guard mktemp failures, sanitize workflow notices, and add bugfix-workflow prerequisites. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Restrict author escape hatch to fix-only, skip leading blank lines, clarify cross-context messaging, pin checkout to default_branch, assert script content parity, and detect stop-agent.sh drift in reconcile. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add ADR 0086 for stop-agent generalization, restore ADR 0034 Decision prose to an annotation, delete stop-agent.sh on unenroll, fix docs, and document the fix-only author escape hatch on #5650. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Document that dispatch still takes the literal first comment line, give stop-agent unenroll its own commit message, and cover /fs-fix-stop on issues in the runtime auth tests. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Treat unenroll as complete only when both the shim and stop-agent.sh are gone, and stop persisting credentials on the stop-agent checkout. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
865e754 to
8c73c1c
Compare
|
🤖 Review · Commit: |
Fold fullsend-no-* gates into existing conditions instead of raising lint-workflow-size past main's 610-line limit. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · Commit: |
Bring the branch up to date with the base branch. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 1:34 PM UTC · Completed 1:48 PM UTC Commit: |
Bring the branch up to date with the base branch.
|
🤖 Finished Review · ✅ Success · Started 10:33 AM UTC · Completed 10:51 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Clarify author-hatch and cancel-deferral in ADR 0086, sanitize COMMENT_USER_LOGIN in workflow commands, and document /fs-fix-stop. Signed-off-by: Adam Scerra <ascerra@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 10:59 AM UTC · Completed 11:16 AM UTC Commit: |
fullsend-ai-review
Bot
removed
the
requires-manual-review
Closing without merge —
|
| #5920 approach | #2888 / ADR 0061 approach |
|---|---|
| Slash command + managed-workflow bash | User applies a label (write permission = auth; no slash ACL matrix) |
Hard-coded fullsend-no-{triage,code,review,fix,retro} in platform dispatch |
Each harness’s CEL trigger includes “label absent” (or equivalent) over NormalizedEvent |
| Only works for agents we wire into dispatch bash | Works for default agents and BYOA — custom harnesses ship their own trigger (building custom agents, BYOA) |
| Deepens bash router debt ahead of cutover | Aligns with #2888 plan: port agents to CEL, then remove bash routing |
How skip-via-#2888 should look (outline, not a new ADR)
When default agents are ported under #2888:
- Convention: pick a small label vocabulary (e.g. one freeze/stop label, or per-agent skip labels — naming TBD in the CEL port work). Applying/removing the label is the user API; no
/fs-stoprequired. - Auth: ADR 0054 stays a platform gate before CEL. Who may apply labels is already forge permission; we don’t invent a second slash-command authorization ladder (the mess feat(#5650): add /fs-stop to skip auto agent triggers #5920 hit with “author may stop fix, not review”).
- Routing: each harness
triggerCEL matches the usual event/command/label entry conditions and excludes the skip/freeze label(s). Example shape (illustrative):eventhas the auto-trigger condition and!"fullsend-stop" in event.labels(exact field paths follow the normative event + agent port PRs in #2888). - BYOA: custom agents opt in by writing the same clause into their harness
trigger— no platform dispatch edit (ADR 0061 goal: custom agents as harness files, not bash edits). - Slash on-demand:
/fs-reviewetc. remain expressible in CEL as today planned; skip labels gate auto paths. Manualfullsend run/ intentional slash can still be allowed when product wants that (same split feat(#5650): add /fs-stop to skip auto agent triggers #5920 wanted, without bash duplication). - Issue→PR inheritance: design when porting agents under #2888 — don’t ship a slash command first and discover it later (open question from sync).
- Not covered by Epic: ADR 0061 — per-repo harness CEL dispatch #2888: killing an already-running job → #5445. Repo-wide disable remains config (customizing agents).
Until CEL cutover, use existing tools: /fs-fix-stop for fix (ADR 0034), repo agent config, and for WIP pin targets close draft PRs / keep branches (agents #11, #86, #87 — work continues on branches).
Other reasons not to land #5920 now
- Auth asymmetry in the proposed author escape hatch is a smell that the slash-command API is the wrong abstraction.
- Growing bash
fullsend-no-*gates is transitional debt #2888 is explicitly trying to delete after agents are ported.
|
🤖 Finished Retro · ✅ Success · Started 12:41 AM UTC · Completed 12:49 AM UTC Commit: |
Retro: PR #5920 —
|
2 participants
Summary
Adds
/fs-stop <agent>(and/fs-fix-stopalias) so humans can applyfullsend-no-*labels that skip auto-triggered agent runs. Dispatch enforces the labels on auto paths; on-demand/fs-<agent>commands still bypass them. In-flight workflow cancellation is intentionally deferred (unsafe substring matching was rejected in review).Related Issue
Addresses #5650 (labeling + dispatch enforcement; cancel follow-up still open)
Changes
stop-fix→stop-agent; support/fs-stop, bare/fs-stop, and/fs-fix-stop/fs-stopapplies labels meaningful for the item type (issue: triage/code; PR: review/fix/retro)fullsend-no-{triage,code,review,fix,retro}on auto paths in both dispatch copies (includingneeds-info→ triage)Testing
go test ./internal/scaffold/ -run 'TestShim|TestDispatch|TestReusable|TestManaged'make linton staged tree in CIChecklist
!for breaking changes)Made with Cursor