Download GitHub attachments before agent runs#428
Conversation
This comment has been minimized.
This comment has been minimized.
Rubrics Review
Notes
Findings
Final Rubric VerdictPASS
|
![sepo-agent-app[bot]](https://avatars.githubusercontent.com/in/3527007?s=60&v=4){kind=small}
| --limit "${RUBRICS_LIMIT}" \ | ||
| --output-file "${RUBRICS_CONTEXT_FILE}" | ||
|
|
||
| - name: Download GitHub attachments |
There was a problem hiding this comment.
This step is prompt enrichment, but it lacks continue-on-error: true while adjacent best-effort enrichment steps use it. If the CLI rejects before writing outputs (mkdirSync, writeFileSync, or setOutput), the composite action fails and the agent never runs on any route. Please make the step non-fatal and let run.ts fall back when the manifest is missing.
| if (promptVars.RUBRICS_CONTEXT_FILE && existsSync(promptVars.RUBRICS_CONTEXT_FILE)) { | ||
| promptVars.RUBRICS_CONTEXT = readFileSync(promptVars.RUBRICS_CONTEXT_FILE, "utf8"); | ||
| } | ||
| if (promptVars.ATTACHMENTS_MANIFEST_FILE && existsSync(promptVars.ATTACHMENTS_MANIFEST_FILE)) { |
There was a problem hiding this comment.
This manifest is only injected into the full/base prompt, but resumed sessions pass buildContinuationPrompt(...) to runAcpx; that continuation prompt does not include ATTACHMENTS_MANIFEST or the localPath instructions. A follow-up /answer or /fix-pr with a private attachment can download the file but still leave the agent with only the original github.com/user-attachments URL. Please include the manifest in continuation prompts, or force full prompt replay when a manifest is present, and add coverage.
| continue; | ||
| } | ||
|
|
||
| const bytes = Buffer.from(await response.arrayBuffer()); |
There was a problem hiding this comment.
await response.arrayBuffer() buffers the whole attachment with no size limit, and this loop has no count cap or fetch timeout. Since URLs are collected from issue/PR/discussion text, a large file, many links, or a slow response can exhaust runner resources or stall this shared pre-agent step. Please add size/count/time limits before downloading arbitrary attachments.
AI Review Synthesis
Summary of PR/IssuePR #428 implements issue #426 by downloading GitHub ReviewOverall, the PR is directionally solid and matches the linked issue plan, but it should not ship until resumed runs receive the attachment manifest. Reviewers disagreed on severity: the rubrics review passed with no warnings, while the code reviews identified concrete runtime issues; the resumed-session gap is supported by the current PR head.
Progress
Issue DetailsResumed sessions lose attachment manifestsCause: Candidate solutions: Include the attachment manifest section in continuation prompts, or force full prompt replay when a manifest exists. Add Comments: This is the main reviewer disagreement: rubrics passed, but the current runtime code confirms the resumed prompt path drops the new manifest context. Attachment enrichment can fail all agent runsCause: The Candidate solutions: Make the step non-fatal with Attachment downloads lack resource boundsCause: Candidate solutions: Add a maximum attachment count, a per-file size cap using Recommended Next StepFIX_PR: The unresolved issues are concrete branch changes suitable for an automated fix-pr pass. Final VerdictNEEDS_REWORK Action Items
|
|
Sepo is dispatching follow-up automation.
Reason: agent planner selected fix-pr: Latest review synthesis for open PR #428 is NEEDS_REWORK with Recommended Next Step FIX_PR; the unresolved items are concrete branch changes and round budget remains available. Task for fix-pr:
Constraints: Ignore optional INFO notes, metadata-only polish, already-fixed findings, and human-judgment nits unless required by those action items. |
|
Sepo could not complete the PR fix run. Inspect the workflow logs and retry if appropriate. |
|
Sepo orchestration needs clarification before it can continue. Sepo cannot continue the automatic PR-fix chain safely. The fix-pr run failed before the agent started because the PR branch's shared action calls
No follow-up workflow was dispatched. Reply with the requested context, then continue with |
|
Ok the first q that i want to ask is, does it only download the file in the latest comment or all past comments? I think this causes a broader issue: what if agents keep searching for all issues for files? We cannot exhaust everything right? But the way seems to provide the environment key to download the file? Or? We are a customized and protected file download command? What to do? |
|
It currently scans all past comments on the current target, not only the latest comment. For PR #428 specifically,
See the current PR code around It does not search all issues in the repo. It only queries the current issue/PR/discussion target number passed into On auth: yes, this is meant to be a customized protected pre-agent download path. The action passes the resolved GitHub token only to the downloader step: What I’d do before merging:
My recommendation: keep current-target scanning, but add the resource limits. Do not add repo-wide issue search for attachments.
|
|
@sepo-agent /answer
Given this, can we go deeper to this idea if "building a customized and protected file download command and share that in the prompt with the agent"? |
|
Yes, we can go deeper, but I’d separate two meanings of “protected command”:
The important caveat: a CLI alone is not a hard security boundary if the agent process also has the raw GitHub token in its environment. In the current runtime, So my recommendation is:
So the short answer is: yes, “customized protected file download command” is a good direction, but for now I’d treat this PR’s pre-agent downloader as the safer first version. A command shared in the prompt should only operate on pre-approved manifest entries, not arbitrary attachment URLs.
|
@sepo-agent /answer can we implement this version directly without the preloaded (we should explore this as a completely direction -- branching out from main and workin on that). |
|
Yes, but “directly without preload” should mean no pre-downloading file bytes, not “no pre-run setup at all”. We still need a pre-agent index/allowlist step, because the protected command needs to know which attachment IDs are allowed. The better branch direction from
The hard part: today So I’d split implementation into two phases:
My recommendation: create a new branch from
|
No I don't think we should do this. We just simplify this:
@sepo-agent /answer |
|
Yes, we can implement this as a new direction from I would still narrow “any given GitHub URL” to any GitHub user-attachment URL: node .agent/dist/cli/download-github-attachment.js \
--url "https://github.com/user-attachments/files/..."The command should:
I would not make it a general authenticated Current architecture fit:
So this version is simpler than PR #428:
I’d close or supersede PR #428 and branch from
|
2 participants
Summary
$RUNNER_TEMP/agent-attachmentsrun-agent-taskto run it before the agent and inject the manifest into the base promptTests
npm run buildnode --test dist/__tests__/attachments.test.js dist/__tests__/envelope.test.js dist/__tests__/workflow-action-validation.test.js dist/__tests__/docs-validation.test.jsgit diff --checkCloses #426