Skip to content

refactor: remove deprecated status-token fallback paths - #2365

Merged
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-cleanup-status-token-deprecations
Jun 17, 2026
Merged

refactor: remove deprecated status-token fallback paths#2365
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-cleanup-status-token-deprecations

Conversation

@ggallen

@ggallen ggallen commented Jun 16, 2026

Copy link
Copy Markdown
Member

Summary

Removes all deprecated status-token fallback paths introduced by #2299, which added on-demand token minting via mint-url and deprecated the static status-token input.

Closes #2299 (deprecation cleanup).

  • Removes the deprecated status-token input from action.yml and all STATUS_TOKEN shell logic (masking, warning, flag forwarding)
  • Removes --status-token flag from fullsend run and the statusToken field from statusOpts
  • Removes --token flag from reconcile-status and the static-token fallback branch
  • Removes deprecated-token tests and updates remaining tests to use mintURL
  • Adds reconcileMintToken and reconcileNewForgeClient test seams with full mint success path tests
  • Removes the deprecated status-token row from docs/reference/installation.md

Breaking-change verification

The composite action (action.yml) is not consumed directly by external repositories. All callers go through the reusable workflows (reusable-code.yml, reusable-fix.yml, reusable-triage.yml, reusable-review.yml, reusable-retro.yml, reusable-prioritize.yml), which were migrated to mint-url in #2299 itself. The action is referenced only from reusable-dispatch.yml via uses: fullsend-ai/fullsend/.github/workflows/reusable-*.yml@v0.

Verification method:

  • grep -rn 'status-token\|statusToken\|STATUS_TOKEN' across all .go, .yml, .yaml, .md, .json, .sh, and .tmpl files — zero results after this change
  • grep -rn 'uses:.*fullsend-ai/fullsend' in .github/workflows/ confirms all consumption goes through the reusable workflow layer, not the composite action directly
  • GitHub code search for status-token in the fullsend-ai org confirms no external references

Test plan

  • go build ./... — compiles cleanly
  • go test ./internal/cli/... — all CLI tests pass (including new mint success path tests)
  • go test ./internal/statuscomment/... — statuscomment tests unaffected
  • go vet ./internal/cli/... ./internal/statuscomment/... — no issues
  • grep -rn 'status-token\|statusToken\|STATUS_TOKEN' — no remnants
  • reconcilestatus.go function coverage at 100%

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

Site preview

Preview: https://1e6ef678-site.fullsend-ai.workers.dev

Commit: 6f7ddf631d4b9d33876cc1c6b8d2fc6ac504789f

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:31 PM UTC · Completed 8:43 PM UTC
Commit: b286324 · View workflow run →

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.37500% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/statuscomment/statuscomment.go 0.00% 4 Missing ⚠️
internal/cli/reconcilestatus.go 93.33% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [test-seam-naming] internal/cli/reconcilestatus.go:16 — The renamed test seam variable reconcileNewForgeClient improves disambiguation over the original newForgeClient, but the package-level variable pattern differs from the instance method pattern (SetClientFactory/InvokeClientFactory) used in statuscomment.go. This is a natural consequence of different code structures (cobra command functions vs struct methods) and is not a defect.
  • [test-seam-pattern-inconsistency] internal/cli/reconcilestatus.go — Package-level test seam variables (reconcileMintToken, reconcileNewForgeClient) use a different pattern from the instance method approach in statuscomment.go. The two patterns serve different architectural contexts: package-level vars for standalone functions, instance methods for struct receivers.
  • [test-seam-pattern-inconsistency] internal/cli/run.gostatusMintToken follows the same package-level test seam pattern as reconcilestatus.go. Same observation as above — consistent within the CLI package but different from the statuscomment package.
  • [method-naming-consistency] internal/statuscomment/statuscomment.go:99InvokeClientFactory is a test-only helper that differs from the existing refreshClient (internal) naming. The name accurately describes its purpose but introduces a new naming pattern.
  • [verification-gap] — The PR description claims all reusable workflows were migrated in fix(#2130): mint fresh tokens for status comments #2299 but does not document the verification methodology. Cross-repo contracts review independently confirmed all seven reusable workflows use mint-url exclusively.
  • [backward-compatible-removal] action.yml — Removal of status-token input. External repos passing status-token will have it silently ignored by GitHub Actions. Status comments will fail unless mint-url is configured. All internal workflows verified as already migrated.
  • [cli-flag-removal] internal/cli/run.go — Removal of deprecated hidden --status-token flag. Direct CLI users passing this flag will get an unknown flag error.
  • [cli-flag-removal] internal/cli/reconcilestatus.go — Removal of deprecated hidden --token flag from reconcile-status command.

Info

  • [edge-case] internal/statuscomment/statuscomment.gostatuscomment.New is now always called with a nil initial client. This is safe because refreshClient is called before every use of n.client and a clientFactory is always set. Future code paths that use n.client without refreshClient would panic, but the current architecture prevents this.
  • [test-seam-documentation] internal/cli/reconcilestatus.go — Package-level test seam variables lack godoc comments explaining their purpose.
Previous run

Review

Findings

Medium

  • [breaking-change-risk] action.yml — Removal of the status-token input from the composite action creates a potential breaking change for external repositories that directly consume fullsend-ai/fullsend@v0 and pass status-token. GitHub Actions silently ignores unknown inputs to composite actions, so existing callers passing status-token will continue to run without errors — however, they will lose the deprecated fallback functionality and status comments will fail if mint-url is not also configured.
    Remediation: Before merging: (1) Verify that all known external repositories consuming this action have been migrated to use mint-url. (2) Consider searching GitHub for uses: fullsend-ai/fullsend to identify external callers.

Low

  • [missing-authorization] No linked issue. The PR references fix(#2130): mint fresh tokens for status comments #2299 as the deprecation origin, which provides implicit authorization for this cleanup. Optionally link to fix(#2130): mint fresh tokens for status comments #2299 for traceability.
  • [scope-verification-needed] action.yml — The PR claims "no callers of the deprecated flags remain" but does not provide detailed evidence of verification. Consider adding a brief note about the verification method (e.g., repo-wide grep, GitHub code search). See also: [breaking-change-risk] finding on this file.

Info

  • [architectural-coherence] internal/cli/run.go:68 — The statusOpts struct removal aligns with the stated intent of removing deprecated status-token functionality.
  • [api-surface-reduction] internal/cli/run.go:113 — Removal of --status-token CLI flag (already marked deprecated and hidden).
  • [api-surface-reduction] internal/cli/reconcilestatus.go:103 — Removal of --token CLI flag (already marked deprecated and hidden).
  • [documentation-coherence] docs/reference/installation.md:736 — Removal of the status-token row is consistent with the claimed scope.
  • [test-coverage-coherence] internal/cli/run_test.go — Deprecated flag tests and the newForgeClient test seam removed correctly.
  • [naming-alignment] action.yml — Shell logic removal aligns with the established mint-url pattern.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 16, 2026
@ggallen
ggallen force-pushed the worktree-cleanup-status-token-deprecations branch from b286324 to 527f45a Compare June 16, 2026 20:47
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 8:51 PM UTC · Ended 9:02 PM UTC
Commit: 4e21a60 · View workflow run →

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. One minor note inline.

Comment thread internal/cli/run_test.go
Remove all deprecated status-token/--token/STATUS_TOKEN code paths
that were superseded by mint-url token minting in PR fullsend-ai#2299. All
workflows were already migrated; this removes the fallback scaffolding.

Signed-off-by: Greg Allen <gallen@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@ggallen
ggallen force-pushed the worktree-cleanup-status-token-deprecations branch from 527f45a to 6f7ddf6 Compare June 16, 2026 21:01
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:06 PM UTC · Completed 9:19 PM UTC
Commit: 6f7ddf6 · View workflow run →

Comment thread internal/cli/reconcilestatus.go
Comment thread internal/statuscomment/statuscomment.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 16, 2026
@ggallen
ggallen added this pull request to the merge queue Jun 17, 2026
Merged via the queue into fullsend-ai:main with commit ed8c416 Jun 17, 2026
17 of 18 checks passed
@ggallen
ggallen deleted the worktree-cleanup-status-token-deprecations branch June 17, 2026 01:34
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:40 AM UTC · Completed 1:49 AM UTC
Commit: 6f7ddf6 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro Analysis: PR #2365

PR: refactor: remove deprecated status-token fallback paths

Timeline

  1. 20:27 UTC — ggallen opens human-authored PR removing deprecated status-token fallback paths (7 files, +149/−131)
  2. 20:28–20:43 — Review agent run #27646019194 completes (~15 min). Posts Low/Info findings about naming consistency and test seam patterns.
  3. 20:47–21:08 — Three review agent runs are dispatched and cancelled as new commits arrive (~21 min of wasted agent compute)
  4. 20:55 — Human reviewer ralphbean approves with one actionable inline suggestion (add parity negative test for --status-token flag removal)
  5. 20:58 — ggallen implements ralphbean's suggestion, pushes commit
  6. 21:07–21:27 — Final review agent run #27648239168 completes (~19 min). Approves with 2 [low]-severity inline comments.
  7. 00:19 — ggallen responds to both agent inline comments, explaining the naming choices are intentional
  8. 01:34 — PR merged

Assessment

Overall: The workflow completed successfully. The human review was higher signal than the agent review — ralphbean identified a genuine test gap, while the agent's findings were style nits that the author dismissed.

Wasted compute: 4 cancelled review runs consumed ~26 minutes of agent time. This is a well-known problem with extensive existing issue coverage.

Low-signal inline comments: The agent posted 2 inline comments at [low] severity that required author effort to dismiss. This added friction without value.

Proposals: None (existing coverage)

All improvement opportunities identified are already tracked by open issues:

  • Cancelled review runs / wasted compute: #981, #1014, #1357, #1418, #1452
  • Low-signal inline comments: #2287 (suppress info-severity inlines), #2333 (configurable severity threshold)
  • Duplicate retro dispatches: #2190 (retro should depend on review completion)

No new proposals filed — the existing issues adequately cover the improvements needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants