Connect installer to Stories 2/7 content (and move enrollment to a repo maintenance workflow in the .fullsend repo) - #264
Conversation
Site previewPreview: https://aa200451-site.fullsend-ai.workers.dev Commit: |
9bb8521 to
76e7b15
Compare
ffbf1de to
12b610e
Compare
|
I ran this against my appdumpster org and the test-repo repo there. You can see:
You'll notice also that the triage agent thinks its a duck. I used this to test out a development workflow. In my local git repo, I can hack on |
waynesun09
left a comment
There was a problem hiding this comment.
Security review: inputs.source_repo validation gap in per-role workflows (triage/code/review.yml). See inline comments for the detailed fix — a three-layer validation step (regex, owner check, config allowlist) should be inserted before the app token is generated.
waynesun09
left a comment
There was a problem hiding this comment.
Security Review — High & Medium Findings
High (blocking)
inputs.source_repo checkout without validation — all three per-role workflows (triage.yml, code.yml, review.yml) checkout inputs.source_repo directly using an app token. Since these are workflow_dispatch workflows, any caller with FULLSEND_DISPATCH_TOKEN can point checkout at an attacker-controlled repo. Fix: add a three-layer validation step (regex, owner check, config.yaml allowlist) before the app token is generated. See inline comment on triage.yml for the full suggested fix.
Medium (non-blocking)
-
workflows: writepermission on fullsend app (types.go) — combined withcontents: write, this allows creating/modifying workflow files in all installed repos. Document the permission escalation and consider scoping the token more narrowly. -
fromJSON(inputs.event_payload)in workflow expressions (triage.yml,code.yml,review.yml) — concurrency keys and env vars parse attacker-controllable JSON from dispatch inputs. Risk is gated by the dispatch token but could enable concurrency group collision or agent prompt manipulation via craftedhtml_url.
waynesun09
left a comment
There was a problem hiding this comment.
Outdated GitHub Actions: actions/checkout (v4 → v6), actions/create-github-app-token (v2 → v3), actions/upload-artifact (v4 → v7). See inline comment for details.
waynesun09
left a comment
There was a problem hiding this comment.
Review — Ralph's fix commit (9879bc6) and overall PR
source_repo validation fix: Looks good
The three-layer validation (regex, org owner check, config.yaml allowlist) is well-designed and correctly placed before token generation. Examined bypass vectors — regex edge cases, TOCTOU, shell injection, yq injection — none exploitable. Error messages are properly sanitized (no raw input echoed). Action version bumps (checkout v6, create-github-app-token v3, upload-artifact v7) are current.
Ralph's decision to accept the fromJSON(inputs.event_payload) residual risk is reasonable — exploitation requires the dispatch token, and the validation now blocks the primary escalation path (arbitrary repo checkout).
ADR-0007 permission escalation note adequately covers the workflows: write risk with three mitigations.
Remaining items (see inline comments)
- Plan doc inclusion —
docs/superpowers/plans/2026-04-17-installer-scaffold-and-repo-maintenance.md(1,287 lines) is an agent implementation guide, not a deliverable. Contains outdated shim patterns with inline expression interpolation. Should be removed from this PR. - PAT credential leak —
e2e/admin/pat.go:482logs the full plaintext token (82 chars) into CI logs via a debuglogfthat captures 100 chars aftergithub_pat_. - jq null handling —
reconcile-repos.shuses.[0].urlwhich returns literal"null"with standalone jq on empty arrays. Currently safe withgh --jq(which suppresses nulls), but// emptyis a defensive hardening.
9879bc6 to
a3227b4
Compare
waynesun09
left a comment
There was a problem hiding this comment.
Overall Review — Round 2
The prior findings (plan doc removal, PAT credential leak, jq null handling) are all resolved. Ralph's three-layer source_repo validation is solid. The scaffold and enrollment reconciliation architecture are well-designed.
New findings from this round
Should fix before merge:
-
set -e+gh apiabort (reconcile-repos.sh:126) — if any repo is inaccessible, the entire reconciliation script aborts instead of skipping that repo. The-zguards are dead code underset -euo pipefail. Add|| truetogh apicalls at lines 126, 134, 238, 246. -
enabled != truetreats omission as disabled (reconcile-repos.sh:204) — repos added to config.yaml without an explicitenabledfield get caught by the unenroll phase. If a repo happens to have a.github/workflows/fullsend.yamlfor unrelated reasons, the script would open a PR to delete it. Useselect(.value.enabled == false)to only unenroll explicitly disabled repos. -
Triage dispatches on
labeledevents (shim-workflow.yaml:27-32) —dispatch-triagematches allissuesevents includinglabeled, causing redundant triage runs on every stage transition (triage→code, code→review). ADR-0002 defines triage triggers asopened,edited, and/triageonly. Add action filtering.
Non-blocking nits:
-
io.ReadAllunbounded (github.go:993) — no size limit on workflow job log reads. Considerio.LimitReader. -
Design spec retains unsafe shim pattern (
2026-04-17-installer-agent-content-design.md) — still shows inline${{ toJSON(github.event) }}instead of the env-var approach used in the actual scaffold. Lower risk than the removed plan doc but could mislead contributors.
Move all deployable content from dispatch/ and Go string constants into internal/scaffold/ as real files. Go's //go:embed bundles the directory into the binary at compile time, keeping YAML as YAML for easier maintenance. Layout mirrors the deployed repo structure: - fullsend-repo/: per-role workflows (triage/code/review), composite action, agent definitions, harness configs, sandbox policies, env configs, and a repo-maintenance workflow - target-repo/: per-role dispatch shim with script-injection prevention (event payload passed via env vars, not inline shell) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WorkflowsLayer now walks scaffold.Content instead of using hardcoded string constants. The managedFiles list is built dynamically from the embedded filesystem — adding a file to internal/scaffold/fullsend-repo/ automatically deploys it on next install. EnrollmentLayer reads shim content from the scaffold instead of an inline Go string. Branch creation errors are logged as info (handles both 422 "already exists" and other transient errors). The codeownersErrorClient test double is simplified by embedding FakeClient instead of manually implementing every interface method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add MergePullRequest, CreateIssue, CloseIssue, ListWorkflowRuns to the forge.Client interface and GitHub implementation. These support the e2e triage dispatch smoke test. Add Variables() to the inference.Provider interface so providers can store non-secret repo variables. The Vertex provider uses this for FULLSEND_GCP_REGION. InferenceLayer.Install() writes variables after secrets. The CLI gets a --gcp-region flag (required with --gcp-project). Fix several bugs in the GitHub client: - base64 content decoding: strip MIME-style line wrapping from GitHub's Contents API response before decoding - response body leaks in CloseIssue and CreateOrUpdateRepoVariable - GetTokenScopes: check status code before parsing headers - dispatch verification now targets triage.yml instead of removed agent.yaml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- WaitForEnter: return non-newline errors instead of swallowing them - Manifest flow error page: set Content-Type header and use generic error message instead of leaking internal error details - HTML-escape app name in success page to prevent XSS - Fix race condition in fakeBrowser test double by using a channel instead of polling a slice Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Phase 2.5 to e2e test: merge enrollment PR, file a test issue, verify the triage workflow dispatches in .fullsend. Filter workflow runs by CreatedAt to avoid false positives from previous runs. PAT creation improvements: - Refresh session before PAT creation (stale CSRF tokens after app flow) - Handle GitHub's custom React dropdowns for permission selection - Multiple token extraction strategies (inputs, clipboard attrs, regex) - Debug logging for token extraction failures Cleanup improvements: - Bulk-delete all fine-grained PATs (handles GitHub's 50-token limit) - Delete shim workflow from test-repo between runs (Phase 2.5 cleanup) - Increase e2e timeout from 4m to 10m for dispatch polling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update normative specs: - ADR 0012: expanded file set from scaffold, per-path requirements for all 12 scaffold files, remove old agent.yaml/repo-onboard.yaml fixtures - ADR 0013: per-role dispatch (triage.yml/code.yml/review.yml) replaces single agent.yaml, updated shim content with event filtering, add repo-maintenance workflow section - ADR 0014: add FULLSEND_GCP_REGION variable Update architecture.md: per-role workflows, repo-maintenance, scaffold. Add design spec and implementation plan documenting the scaffold approach, repo-maintenance workflow design, and e2e test extension. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace enroll-repos.sh with reconcile-repos.sh that handles both enrollment (enabled repos) and unenrollment (disabled repos). When a repo is set to enabled: false in config.yaml, the reconciliation workflow now creates a PR to remove the shim workflow from that repo. Changes: - New reconcile-repos.sh with Phase 1 (enroll) and Phase 2 (unenroll), repo name validation, cross-direction PR cleanup, and SHA-based file deletion via the GitHub Contents API - Add config.DisabledRepos() method mirroring EnabledRepos() - Update EnrollmentLayer to accept disabledRepos, fix early return when only disabled repos need reconciliation, and expand Analyze to detect stale shims on disabled repos - Add e2e Phase 2.75 testing the full unenrollment flow: disable repo in config, dispatch reconciliation, verify removal PR, merge it, and confirm the shim is gone - Update normative specs (ADR-0012, ADR-0013) for the new script name, unenrollment constants, and bidirectional behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The shim workflow template existed in two places: target-repo/ scaffold and fullsend-repo/templates/shim-workflow.yaml. Collapse to a single copy in templates/ and update all docs and tests to match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The manifest flow callback redirect timeout (10s) was too tight for GitHub's app creation, which can be slow. Increased to 30s. Also added a 2-minute context deadline per role in the app setup loop so that failures produce a clear error instead of hanging until the Go test timeout panics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add three-layer validation (regex, org owner check, config.yaml allowlist) to triage, code, and review workflows. The step runs after .fullsend checkout but before app token generation, so a forged workflow_dispatch can never mint a token for an unauthorized repo. Also bump actions/checkout v4→v6, actions/create-github-app-token v2→v3, actions/upload-artifact v4→v7 across all scaffold workflows, and document the workflows:write permission escalation risk in ADR-0007. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Wayne Sun <ericbreeze@gmail.com>
0dcf5d6 to
7e2c410
Compare
waynesun09
left a comment
There was a problem hiding this comment.
All prior findings resolved. Latest commit (7e2c4107) correctly addresses the set -e trap (|| true on all gh api --jq calls) and enabled == false semantics.
One non-blocking item remains: dispatch-triage in the shim fires on labeled events, causing redundant triage runs on stage transitions (see inline comment on shim-workflow.yaml). Can be addressed in a follow-up.
LGTM.
Summary
enabled: falseinconfig.yaml, therepo-maintenance.ymlworkflow now creates a PR to remove the shim workflow from that repo (via GitHub Contents API DELETE with blob SHA). Stale cross-direction PRs are automatically closed.internal/scaffold/target-repo/intointernal/scaffold/fullsend-repo/templates/shim-workflow.yaml— one copy, no sync test needed..fullsendrepo content withWalkFullsendRepofor filesystem-order deployment.DispatchWorkflow,ListWorkflowRuns,AwaitWorkflowRun,DeleteFile,ListPullRequestsmethods added.::error::annotations), yq query fix (enabled != trueto match Go zero-value semantics).Test plan
make go-test— all unit tests passmake go-vet— cleanDisabledRepos()returns sorted disabled repos🤖 Generated with Claude Code