-
Notifications
You must be signed in to change notification settings - Fork 61
refactor(harness): migrate code agent to env.runner/env.sandbox (ADR 0055) #2760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,20 +41,35 @@ plugins: | |
|
|
||
| # Environment variables available to pre/post scripts on the runner. | ||
| # These are expanded from the runner environment and NEVER enter the sandbox. | ||
| runner_env: | ||
| CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}" | ||
| FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/code-result.schema.json | ||
| FULLSEND_OUTPUT_FILE: code-result.json | ||
| env: | ||
| runner: | ||
| CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}" | ||
| FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/code-result.schema.json | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] lint-diagnostic-expected code.yaml now declares env.sandbox alongside host_files delivering code-agent.env. Both inject into sandbox but the .env file only contains the GIT_SSL_CAINFO shell conditional that env.sandbox cannot express. |
||
| FULLSEND_OUTPUT_FILE: code-result.json | ||
| sandbox: | ||
| MAX_RETRIES: "1" | ||
| TIMEOUT_SECONDS: "2100" | ||
| GOPATH: "/sandbox/go" | ||
| GOMODCACHE: "/sandbox/go/pkg/mod" | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
|
|
||
| timeout_minutes: 35 | ||
|
|
||
| forge: | ||
| github: | ||
| pre_script: scripts/pre-code.sh | ||
| post_script: scripts/post-code.sh | ||
| runner_env: | ||
| PUSH_TOKEN: "${PUSH_TOKEN}" | ||
| PUSH_TOKEN_SOURCE: "${PUSH_TOKEN_SOURCE}" | ||
| REPO_FULL_NAME: "${REPO_FULL_NAME}" | ||
| ISSUE_NUMBER: "${ISSUE_NUMBER}" | ||
| REPO_DIR: "${GITHUB_WORKSPACE}/target-repo" | ||
| env: | ||
| runner: | ||
| PUSH_TOKEN: "${PUSH_TOKEN}" | ||
| PUSH_TOKEN_SOURCE: "${PUSH_TOKEN_SOURCE}" | ||
| REPO_FULL_NAME: "${REPO_FULL_NAME}" | ||
| ISSUE_NUMBER: "${ISSUE_NUMBER}" | ||
| REPO_DIR: "${GITHUB_WORKSPACE}/target-repo" | ||
| sandbox: | ||
| ISSUE_NUMBER: "${ISSUE_NUMBER}" | ||
| GITHUB_ISSUE_URL: "${GITHUB_ISSUE_URL}" | ||
| GH_TOKEN: "${GH_TOKEN}" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct. If this gets expanded within the sandbox, there is no GH_TOKEN within the sandbox right? That is why GH_TOKEN was on a file expanded at runner level. Now I'm not sure if other similar PRs to this one are OK. Same for |
||
| GIT_AUTHOR_NAME: "fullsend-code" | ||
| GIT_AUTHOR_EMAIL: "${GIT_BOT_EMAIL}" | ||
| GIT_COMMITTER_NAME: "fullsend-code" | ||
| GIT_COMMITTER_EMAIL: "${GIT_BOT_EMAIL}" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] test-coverage-gap
TestResolveForge_ScaffoldRunnerEnvMerge validates only runner-side env keys after forge resolution. The diff adds eleven sandbox variables but no test asserts that h.Env.Sandbox contains the expected keys after resolution. A regression in sandbox env merging would go undetected for the code agent.
Suggested fix: Add a sandboxKeys field to the code.yaml test case and assert that h.Env.Sandbox contains the expected keys after forge resolution.