Skip to content

fix: revert PR #142, merge correct admin CLI (PR #160), add e2e tests - #166

Closed
ralphbean wants to merge 47 commits into
mainfrom
agent-revert-142-merge-160-e2e-squashed
Closed

fix: revert PR #142, merge correct admin CLI (PR #160), add e2e tests#166
ralphbean wants to merge 47 commits into
mainfrom
agent-revert-142-merge-160-e2e-squashed

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary\n\nPR #142 was merged by mistake. This PR:\n\n1. Reverts PR #142 — removes the incorrectly merged admin CLI\n2. Merges PR #160 — the correct v6 admin CLI with dispatch token layer, preflight checks, idempotent operations, ADRs, and GoReleaser\n3. Adds e2e tests (from PR #158) adapted for v6, including fine-grained PAT automation via Playwright\n\n## E2E test details\n\nThe e2e tests automate the complete admin install/uninstall lifecycle via Playwright browser automation against real GitHub. The dispatch token is provisioned as a fine-grained PAT (matching production flow) rather than cheating with a classic PAT.\n\n## Verification\n\n- go test -race ./... — all unit tests pass\n- make e2e-test — full e2e suite passes (~170s)\n- go vet -tags e2e ./e2e/admin/ — clean"

ralphbean added 30 commits April 2, 2026 03:03
Assisted-by: OpenCode claude-opus-4-6@default
Assisted-by: OpenCode claude-opus-4-6@default
Add the ui.Printer type with lipgloss-styled methods for consistent
terminal output across the CLI: banner, headers, step indicators,
key-value pairs, summary/error boxes, and PR links. Includes full
test coverage for all 12 methods.

Assisted-by: OpenCode claude-opus-4-6@default
The forge.Client interface abstracts all git forge operations, enabling
future support for GitHub, GitLab, and Forgejo.

Assisted-by: OpenCode claude-opus-4-6@default
Implements all forge.Client methods against the GitHub REST API including
repo management, file operations, secret encryption, and workflow queries.
Includes GitHub-specific types for App configuration with role-based presets.

Assisted-by: OpenCode claude-opus-4-6@default
Handles OrgConfig types, YAML marshal/unmarshal, validation, and
helper methods for accessing enabled repos and agent slugs.

Assisted-by: OpenCode claude-opus-4-6@default
Layers represent discrete installation concerns processed in order for
install, reverse order for uninstall, and assessed individually for analyze.

Assisted-by: OpenCode claude-opus-4-6@default
Handles creation, configuration, and teardown of the org-level
.fullsend configuration repository. The layer creates the repo
(private or public based on org capability), writes config.yaml,
and provides analysis of existing installation state.

Assisted-by: OpenCode claude-opus-4-6@default
Manages reusable agent dispatch workflow, onboarding workflow, and
CODEOWNERS in the .fullsend config repo.

Assisted-by: OpenCode claude-opus-4-6@default
Stores agent app private keys as repo secrets and app IDs as repo
variables in the .fullsend config repo.

Assisted-by: OpenCode claude-opus-4-6@default
Creates enrollment PRs with shim workflow files for enabled repos
that are not yet connected to the fullsend agent pipeline.

Assisted-by: OpenCode claude-opus-4-6@default
Handles creating and installing per-role GitHub Apps using the
manifest flow, with support for reusing existing apps.

Assisted-by: OpenCode claude-opus-4-6@default
Implements fullsend admin {install,uninstall,analyze} <org> with
layer-based installation model and forge-agnostic client interface.

- Root command with Cobra, version support, and silence flags
- Admin subcommand grouping install, uninstall, and analyze
- Install: app setup, repo discovery, layer stack creation and execution
- Uninstall: confirmation prompt, layer teardown, manual cleanup hints
- Analyze: layer-by-layer status assessment with actionable reporting
- Token resolution from GH_TOKEN, GITHUB_TOKEN, or gh CLI
- Org name validation
- Dry-run mode for install preview

Assisted-by: OpenCode claude-opus-4-6@default
Critical fixes:
- Fix XSS vulnerability in app manifest HTML form (html.EscapeString)
- Add retry with backoff on rate-limited responses (429 + 403 w/ Retry-After)
- Fix exchangeManifestCode to use context-aware HTTP client with timeout
- Add GetRepo to forge.Client for O(1) repo existence checks

Important fixes:
- Add forge.IsNotFound/ErrNotFound for structured error discrimination
- Fix runUninstall to return error when uninstall operations fail
- Secrets layer Analyze now checks both secrets and variables
- Move DefaultAgentRoles to config package to decouple CLI from forge/github
- Fix FakeClient embedding by pointer in enrollment test (mutex safety)
- Add shared forge.ConfigRepoName constant used across all packages
- Fix go.mod to use go 1.25.8

Minor fixes:
- Use errors.As in isNotFound instead of manual unwrap loop
- Add pagination to ListRepoPullRequests
- Delete pointless TestCompileTimeInterfaceCheck
- Add bin/ to .gitignore

Assisted-by: OpenCode claude-opus-4-6@default
Documents non-obvious GitHub API behaviors discovered during the original
implementation:
- auto_init is async; file writes after repo creation need retry
- Contents API requires existing file SHA for updates (422 otherwise)
- Sequential file writes cause transient 404s as branch refs update
- Writing to .github/workflows/ returns 404 (not 403) without workflow scope
- App PEM private keys are one-shot; only available at creation time
- Event subscriptions must have matching permissions or manifest is rejected
- App installation URL must not include target_id parameter
- Org-scoped app settings need /advanced suffix in URL
- App uninstall API requires JWT auth, not PAT (browser fallback needed)
- Users can rename apps during creation; match by stored slug first
- Token scopes (delete_repo, workflow) are often missing from default gh auth

Assisted-by: OpenCode claude-opus-4-6@default
Preflight checks: each layer declares the OAuth scopes it needs for
install, uninstall, and analyze via RequiredScopes(). Before running
any operation, the CLI queries the token's scopes (via X-OAuth-Scopes
header) and fails early with remediation instructions if scopes are
missing. This prevents confusing mid-operation failures like the 403
on repo deletion when delete_repo scope is absent.

Auto-reuse apps: when an existing GitHub App is found with its PEM
secret still stored, it is now reused automatically without prompting.
The previous confirm prompt was inconsistent with how other resources
(repos, workflows, secrets) are handled — those are silently reused.
To get fresh apps, uninstall first, then reinstall.

Assisted-by: OpenCode claude-opus-4-6@default
Instead of printing URLs for the user to copy-paste, uninstall now
opens the browser directly to each app's advanced settings page
(/organizations/{org}/settings/apps/{slug}/advanced) where the
'Delete GitHub App' button lives.

Assisted-by: OpenCode claude-opus-4-6@default
ConfigRepoLayer.Uninstall: checks if repo exists before deleting;
if already gone, logs and proceeds. Also handles the race where the
repo is deleted between the check and the delete call.

ConfigRepoLayer.Install: if CreateRepo fails, re-checks existence
to handle 'already exists' errors from races or repeated runs.

EnrollmentLayer.Install: treats CreateBranch errors as non-fatal
(branch may exist from a previous partial run). Also checks for
existing enrollment PRs before creating duplicates.

All layers were already idempotent for their other operations:
- WorkflowsLayer uses CreateOrUpdateFile (upsert)
- SecretsLayer uses CreateRepoSecret/CreateOrUpdateRepoVariable (upsert)
- All no-op Uninstall methods are trivially idempotent

Assisted-by: OpenCode claude-opus-4-6@default
When the .fullsend repo has already been deleted (e.g., partial
uninstall), the uninstall command can no longer read config.yaml
to find the actual app slugs. Previously this caused the app
cleanup step to be silently skipped, leaving orphaned apps that
block reinstallation (PEM keys are only available at creation time).

Now falls back to the default naming convention (fullsend-{org},
fullsend-{org}-triage, etc.) so the browser is still opened to
the correct deletion pages. Also exports ExpectedAppSlug for use
outside the appsetup package.

Assisted-by: OpenCode claude-opus-4-6@default
GitHub's manifest flow requires redirect_url and hook_attributes to
be inside the JSON manifest body, not as separate form fields. The
previous code sent redirect_url as its own hidden input, which GitHub
rejected with 'redirect_url wasn't supplied'.

Also adds hook_attributes with active:false to the manifest — GitHub
requires this field even when webhooks are not used.

Assisted-by: OpenCode claude-opus-4-6@default
All app names now follow the uniform pattern <org>-<role>:
  apache-fullsend, apache-triage, apache-coder, apache-review

Previously the fullsend role used 'fullsend-<org>' while others used
'fullsend-<org>-<role>', which was inconsistent and made the
orchestrator app name ambiguous for orgs with common names.

Assisted-by: OpenCode claude-opus-4-6@default
GitHub's auto_init is asynchronous — CreateRepo returns 201 before
the default branch is fully materialized. The Contents API returns
404 until the initial commit lands. Sequential file writes can also
hit 409 (conflict) as the branch ref updates between commits.

CreateOrUpdateFile and CreateFileOnBranch now retry up to 5 times
with 2s linear backoff on 404 and 409 errors. Non-transient errors
(permission denied, validation errors) fail immediately.

Assisted-by: OpenCode claude-opus-4-6@default
When a previous install partially completed, the enrollment branch
and shim workflow file may already exist. CreateFileOnBranch returns
422 'sha wasn't supplied' in this case. Now treats that as 'file
already present' and proceeds to PR creation.

Assisted-by: OpenCode claude-opus-4-6@default
Adds CreateOrgSecret, OrgSecretExists, DeleteOrgSecret, and
SetOrgSecretRepos to forge.Client for managing org-level Actions
secrets. Also adds ID field to forge.Repository for scoping
org secrets to selected repositories.

Assisted-by: OpenCode claude-opus-4-6@default
The DispatchTokenLayer manages FULLSEND_DISPATCH_TOKEN, an org-level
Actions secret that enrolled repos use to trigger workflow_dispatch
events on the .fullsend config repo. This replaces the previous
model where App private keys were passed via workflow_call secrets.

Assisted-by: OpenCode claude-opus-4-6@default
The agent dispatch workflow now uses workflow_dispatch instead of
workflow_call. Shim workflows in enrolled repos trigger dispatch via
curl using FULLSEND_DISPATCH_TOKEN (an org-level secret), rather
than passing App private keys via workflow_call secrets. This ensures
private keys never leave the .fullsend repo.

The shim uses pull_request_target instead of pull_request to prevent
malicious PRs from modifying the workflow to exfiltrate the dispatch
token.

Assisted-by: OpenCode claude-opus-4-6@default
The install flow now prompts for a fine-grained PAT (or reuses an
existing one) and stores it as the FULLSEND_DISPATCH_TOKEN org secret.
Uninstall deletes it. Analyze checks for its existence.

Assisted-by: OpenCode claude-opus-4-6@default
…h security

Records architectural decisions made on the admin-cli-clean-room branch:
- ADR 0004: Forge abstraction layer
- ADR 0005: Ordered layer model
- ADR 0006: Per-role GitHub Apps
- ADR 0007: workflow_dispatch for cross-repo dispatch
- ADR 0008: pull_request_target in shim workflows

Updates architecture.md and agent-architecture.md to reflect decisions.

Assisted-by: OpenCode claude-opus-4-6@default
The preflight scope check now runs before promptDispatchToken so that
a missing admin:org scope is caught with clear remediation instructions
before the user is asked to paste a PAT.

Also makes OrgSecretExists treat 403 as 'unknown' (returns false)
instead of a hard error. This handles the case where preflight can't
introspect scopes (fine-grained tokens) — the operation proceeds and
fails at the actual CreateOrgSecret call with a clear error.

Assisted-by: OpenCode claude-opus-4-6@default
Instead of asking the user to manually fill out the fine-grained PAT
form, we now open the browser to GitHub's token creation page with
name, description, resource owner, and actions:write permission
pre-filled via URL query parameters. The user only needs to:
  1. Select 'Only select repositories' and pick .fullsend
  2. Click 'Generate token'
  3. Paste the result

Assisted-by: OpenCode claude-opus-4-6@default
ralphbean and others added 17 commits April 2, 2026 17:11
When re-running install, the enrollment layer now updates the shim
workflow content on existing enrollment branches instead of skipping
repos with open PRs. This ensures PRs always reflect the latest shim
(e.g., after switching from workflow_call to workflow_dispatch).

Adds CreateOrUpdateFileOnBranch to the forge interface — combines
SHA-aware upsert with branch targeting. Also adds PullRequests field
to FakeClient for pre-populating open PRs in tests.

Assisted-by: OpenCode claude-opus-4-6@default
Uses the gh CLI (pre-installed on GitHub-hosted runners) instead of
raw curl for dispatching. Cleaner, no manual JSON escaping, and auth
is handled via GH_TOKEN env var.

Assisted-by: OpenCode claude-opus-4-6@default
After the user pastes the PAT, we now make a test API call to the
.fullsend repo using the token. If the PAT was created with the
wrong repo selected (easy to do since GitHub can't pre-fill repo
selection via URL params), this catches it immediately with a clear
error message instead of silently storing a broken token that fails
on every future dispatch.

Also improved the step-by-step instructions to be more explicit
about selecting ONLY the .fullsend repository.

Assisted-by: OpenCode claude-opus-4-6@default
After opening the browser for app installation, the CLI now polls
ListOrgInstallations every 2 seconds until the app appears (up to
5 minutes). The user installs the app in the browser and the CLI
proceeds automatically — no need to switch back to the terminal
and press Enter.

Assisted-by: OpenCode claude-opus-4-6@default
During uninstall, check ListOrgInstallations to confirm each app
slug is real before opening the browser. Apps that don't exist are
logged and skipped. Falls back to opening all if the installations
API call fails.

Assisted-by: OpenCode claude-opus-4-6@default
The fine-grained PAT creation UI requires the user to select which
repos the token can access. If .fullsend doesn't exist yet, the user
can't select it. Now the config repo layer runs first (creating the
repo and writing config.yaml), then the PAT prompt opens. The full
layer stack still runs afterward — the config repo layer is idempotent
so the second pass is a no-op.

Assisted-by: OpenCode claude-opus-4-6@default
The previous verification used GetRepo which only checks metadata:read,
a permission implicitly granted to all org repos. A PAT without
.fullsend explicitly selected would pass. Now uses GetLatestWorkflowRun
which requires actions:read/write on the specific repo — catches
misconfigured PATs before storing them.

Assisted-by: OpenCode claude-opus-4-6@default
Aggressively strips \r, \n, and whitespace from the pasted token
at both the CLI input layer and the encryption layer (defense in
depth). Pasting from a browser can introduce invisible characters
that corrupt the token when stored as a GitHub Actions secret.

Also applies TrimSpace to CreateRepoSecret for consistency.

Assisted-by: OpenCode claude-opus-4-6@default
Instead of checking actions:read (which passes even with wrong PAT
config), the verification now attempts an actual workflow_dispatch
on agent.yaml in .fullsend. This is the exact operation the shim
will perform, so if verification passes, the shim will work.

Also writes workflow files before the PAT prompt so agent.yaml
exists when we attempt the test dispatch.

Adds DispatchWorkflow to forge.Client interface with GitHub and
fake implementations.

Assisted-by: OpenCode claude-opus-4-6@default
GitHub's workflow dispatch API returns 204 (not 200/201) on success.
The post() helper only accepted 200 and 201, causing the verification
to fail with 'HTTP 204' error even when the dispatch succeeded.

Now uses do() + checkStatus(204) directly instead of post().

Assisted-by: OpenCode claude-opus-4-6@default
When a semver tag (v*) is pushed, the release workflow cross-compiles
the fullsend binary for linux and darwin (amd64/arm64), generates a
changelog, and publishes a GitHub Release with the binaries attached.

Also fixes the ldflags path in the Makefile to target the correct
package variable (internal/cli.version instead of main.version).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat: add release workflow with GoReleaser
…n-room"

This reverts commit 3eae7d3, reversing
changes made to 3519563.
…n CLI implementation

Merges the correct admin CLI implementation from PR #160 after reverting
the incorrectly merged PR #142. The v6 branch includes:
- Dispatch token layer for org-level cross-repo dispatch
- Preflight scope checks and auto-reuse of existing apps
- Idempotent layer operations
- ADRs for forge abstraction, layer model, app model, dispatch security
- GoReleaser release workflow
- Numerous bug fixes and improvements

Assisted-by: OpenCode claude-opus-4-6@default
…en automation

Implements end-to-end tests for the admin install/uninstall/analyze flow,
adapted to work with the v6 admin CLI from PR #160 which adds the
DispatchTokenLayer.

Test flow (mirrors production CLI):
1. Create 4 GitHub Apps via manifest flow (fullsend, triage, coder, review)
2. Install apps on the org via Playwright browser automation
3. Pre-install config-repo + workflows layers (so .fullsend repo exists)
4. Create fine-grained PAT scoped to .fullsend with Actions permission
   via Playwright (automating GitHub's PAT creation UI)
5. Install full layer stack including DispatchTokenLayer
6. Verify all resources exist (secrets, variables, org secret, enrollment PR)
7. Re-install idempotently (verify no-op behavior)
8. Uninstall all layers and verify cleanup
9. Re-uninstall idempotently (verify not-found handling)

Key implementation details:
- Fine-grained PAT creation handles GitHub's resource owner selector quirk
  (must manually select org even when pre-filled via query param)
- Added CLI warning about this quirk for human users
- Browser automation handles 404 retries on freshly created apps
- Cleanup handles both old (fullsend-<org>) and v6 (<org>-<role>) app naming
- Cleanup deletes apps by expected slug (catches uninstalled apps not in
  ListOrgInstallations)
- Screenshots saved to .playwright/ for debugging
- Dispatch PAT names include timestamp to avoid collisions
- Delays after repo deletion to handle GitHub's async propagation

Assisted-by: OpenCode claude-opus-4-6@default
@ralphbean

Copy link
Copy Markdown
Member Author

Moving to #167

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant