feat: code agent automation layer - #208
Conversation
|
The e2e failure here is expected — GitHub does not expose secrets to |
ben-alkov
left a comment
There was a problem hiding this comment.
Review: #208
Head SHA: 794047d4a95833ff05317206a464fc7d546b69e2
Timestamp: 2026-04-09T16:30:00Z
Outcome: request-changes
Summary
The PR implements the authorized automation layer (issue #127 task #3) correctly in structure and intent. Token isolation is substantially sound — the bot token is stripped from the agent environment via SanitizeEnv. However, two findings require resolution. First, several GHA runner environment variables (GITHUB_ENV, GITHUB_OUTPUT, GITHUB_PATH, ACTIONS_RUNTIME_TOKEN, ACTIONS_CACHE_URL) pass through SanitizeEnv unchanged; a prompt-injected code agent could write to GITHUB_ENV/GITHUB_OUTPUT to inject variables into subsequent pipeline steps. Second, the commit-detection logic treats any non-zero git diff --quiet exit as "commits exist," meaning exit 128 (infrastructure error) would let the pipeline proceed past the commit check on a branch with no agent-produced changes.
Findings
High
-
[content-security / token-leakage-via-gha-sinks]
internal/entrypoint/runner.go:40-65—SanitizeEnvremovesFULLSEND_*,GITHUB_TOKEN, andGH_TOKEN, but does not stripGITHUB_ENV,GITHUB_OUTPUT,GITHUB_PATH,GITHUB_STEP_SUMMARY,ACTIONS_RUNTIME_TOKEN, orACTIONS_CACHE_URL. These are file paths and tokens that GHA watches: any process that appendsKEY=VALUEto the file atGITHUB_ENVinjects that variable into every subsequent step in the same job.ACTIONS_RUNTIME_TOKENgrants access to the GHA artifact and cache APIs.
Remediation: Extend the deny list to includeGITHUB_ENV,GITHUB_OUTPUT,GITHUB_PATH,GITHUB_STEP_SUMMARY,GITHUB_STATE,ACTIONS_RUNTIME_TOKEN,ACTIONS_CACHE_URL, and add a deny prefixACTIONS_. Alternatively, invert to an allowlist of explicitly safe variables (PATH,HOME,LANG,TERM,TMPDIR). -
[logic-error / false-positive-commit-detection]
internal/entrypoint/code.go:113-120—git diff --quietexit code!= 0is interpreted as "commits exist," but exit 128 indicates a git infrastructure error (bad ref, corrupt repo). This would let the pipeline proceed to secret scan and push with no agent commits.
Remediation: Check specifically for exit code 1 as "has commits." Treat any other non-zero code as an infrastructure failure:switch diffCode { case 0: // no commits case 1: // has commits, proceed default: return nil, fmt.Errorf("git diff exited %d", diffCode) }
Medium
-
[data-exposure / credential-at-rest]
internal/entrypoint/code.go:159-170—git remote set-url origin https://x-access-token:<token>@github.com/...writes the bot token into.git/config. After push, the token persists on disk. GHA ephemeral runners mitigate this, but it is unnecessary residual risk.
Remediation: After push, reset the remote URL to the non-authenticated form, or usegit push https://x-access-token:<token>@... HEAD:<branch>as a one-shot argument. -
[observability / silent-stderr]
internal/entrypoint/runner.go:29-31—ExecRunnercaptures stderr into a buffer that is never used. When commands fail, operators get only the exit code with no diagnostic content.
Remediation: Includestderr.String()in the error for non-zero exits. -
[missing-test / token-isolation]
internal/entrypoint/code_test.go— No test verifies that thesafeEnvslice passed to the claude runner excludesenv.BotToken.SanitizeEnvis tested in isolation, butRunCodetests passnilforsafeEnv.
Remediation: Add a test that constructs a dirty environment and asserts the agent runner call'sEnvfield excludes bot token andGITHUB_TOKEN.
Low
- [style / misleading-description]
internal/cli/entrypoint.go:36— Long description says "Clones the repo" butRunCodeexpects a pre-cloned workspace (env.Workspace).
Remediation: Change to "Runs the code agent against a pre-checked-out workspace, validates output, and opens a PR."
Info
- [intent-alignment] The PR implements task #3 from the sync-up on #127. Scope matches authorization. CI-failure retry and dispatch routing are correctly deferred to Story 9 and Story 2 respectively.
- [injection-defense] No non-rendering Unicode found in PR body, commit messages, or code. Agent prompt strings embed only integer issue numbers and internal branch/ref names — no untrusted user content reaches prompt strings at the automation layer.
Footer
Outcome: request-changes
This review applies to SHA 794047d4a95833ff05317206a464fc7d546b69e2. Any push to the PR head clears this review and requires a new evaluation.
|
The preceding is from the review agent in PR #171. It had a pertinent note, and a summary of the review as a test:
|
ccfe6b4 to
baf6931
Compare
maruiz93
left a comment
There was a problem hiding this comment.
I believe this overlaps with #231
I think that if my PR is merged this should probably be refactored into:
- pre-scripts for the git setup
- validation scan for the scan of the output
- post-script for the gh push
Then, for the rest of the gh integration it should be probably keep being integrated in the fullsend cli, and in a way it's used with the run command.
Also, the review agent could be a separate agent with its own harness in the "framework" I defined.
Addresses review feedback from waynesun09 on PR #189. disallowedTools (H1, M1): Add bare-form (zero-arg) variants for all blocked commands, add gh api blocking, add git reset --hard and git rebase. Documented that disallowedTools is belt-and-suspenders — sandbox network policy and post-script validation are the load-bearing layers. scan-secrets (H3, M3, L1): Pin gitleaks version with SHA256 checksum verification for all supported platforms. Add tar extraction error handling. Fix pre-commit fallback to require the gitleaks-specific hook rather than falling through to generic hooks. Add sha256sum/shasum portability for macOS. architecture.md (M2, I2): Document the four-layer defense-in-depth model, three-layer secret scanning architecture, and protected-path enforcement via post-script. skill (M4, I3, L2, L3): Add MAX_RETRIES env var for retry limit. Add multi-run branch reuse guidance for review-rejected commits. Add partial work section. Remove constraint duplication — skill defers to agent definition as authoritative. H2 (protected-path enforcement in post-script) deferred to PR #208. Sandbox network policy and harness timeout deferred to PR #231. Signed-off-by: Adam Scerra <ascerra@redhat.com> Made-with: Cursor
Five new methods for the code agent automation layer: - AddIssueLabel, RemoveIssueLabel, AddIssueComment - FindOpenPRByHead, CreateDraftChangeProposal Also adds Head and Draft fields to ChangeProposal. Related: fullsend-ai#127 Assisted-by: Claude Code (Opus 4.6)
Implements `fullsend entrypoint code` — the deterministic wrapper that orchestrates the code agent lifecycle: configure git identity, run the code agent, check for commits, scan for secrets, optionally run a pre-push review agent, push the branch, and open a draft PR. Gracefully degrades when agents/review.md is absent (skips review). Token isolation: bot token stays in Go code, never enters the agent's environment. Related: fullsend-ai#127 Assisted-by: Claude Code (Opus 4.6)
baf6931 to
4526641
Compare
Site previewPreview: https://b01bcb31-site.fullsend-ai.workers.dev Commit: |
|
Closing with a link to #286 after discussion in slack. |
Summary
forge.Clientmethods for issue labels, comments, and PR lookup/creation (prep commit)fullsend entrypoint code— the deterministic Go wrapper that orchestrates the code agent lifecycle (env parsing, git setup, code agent, commit check, secret scan, optional pre-push review, push, draft PR, label swap)agents/review.mdis absent; token isolation ensures bot credentials never enter the agent's environmentAddresses task #3 from the sync-up on #127, assigned to @ben-alkov.
Test plan
go vet ./...cleanpre-commitcleanfullsend entrypoint code --scm github)