Skip to content

fix(#349): use runner_env YAML key matching Go struct tags#502

Open
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/349-fix-harness-runner-env
Open

fix(#349): use runner_env YAML key matching Go struct tags#502
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/349-fix-harness-runner-env

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

Harness YAML files used nested env.runner for runner environment variables, but Harness.RunnerEnv and ForgeConfig.RunnerEnv are tagged yaml:"runner_env" and expect a flat runner_env key. The YAML parser silently ignored the env.runner sections, leaving RunnerEnv nil after unmarshalling.

This caused REVIEW_TOKEN (and other forge-specific runner env vars like PUSH_TOKEN, GH_TOKEN) to never reach post-scripts via the harness config. Post-scripts fell back to inheriting vars from os.Environ() via the GHA step env, which breaks when the workflow identity matches the PR author (GitHub rejects self-reviews with 422).

Changes all harness YAML files (scaffold defaults and customized overrides) from nested env.runner to flat runner_env matching the Go struct tags. Also removes env.sandbox sections which were equally silently ignored (no SandboxEnv field exists in Harness or ForgeConfig). Fixes code.yaml test expectations to match actual runner_env keys.


Closes #349

Post-script verification

  • Branch is not main/master (agent/349-fix-harness-runner-env)
  • Secret scan passed (gitleaks — b776cf6006a12016eaa21412dc65a3001100e801..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

Harness YAML files used nested env.runner for runner environment
variables, but Harness.RunnerEnv and ForgeConfig.RunnerEnv are
tagged yaml:"runner_env" and expect a flat runner_env key. The
YAML parser silently ignored the env.runner sections, leaving
RunnerEnv nil after unmarshalling.

This caused REVIEW_TOKEN (and other forge-specific runner env
vars like PUSH_TOKEN, GH_TOKEN) to never reach post-scripts
via the harness config. Post-scripts fell back to inheriting
vars from os.Environ() via the GHA step env, which breaks when
the workflow identity matches the PR author (GitHub rejects
self-reviews with 422).

Changes all harness YAML files (scaffold defaults and
customized overrides) from nested env.runner to flat runner_env
matching the Go struct tags. Also removes env.sandbox sections
which were equally silently ignored (no SandboxEnv field
exists in Harness or ForgeConfig). Fixes code.yaml test
expectations to match actual runner_env keys.

Closes #349
@github-actions

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 14, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:14 PM UTC · Completed 3:24 PM UTC
Commit: b776cf6 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review — PR #502

Verdict: Comment · 1 medium finding

Summary

Correct root-cause fix for #349. The harness YAML files used env: runner: (nested) which doesn't match the Go struct tag yaml:"runner_env" — the YAML parser silently produced nil RunnerEnv maps, preventing REVIEW_TOKEN and other forge-specific vars from reaching post-scripts. Converting to flat runner_env: matches the struct tags and aligns all harness files with code.yaml, which was already correct.

The env: sandbox: removal is also valid — confirmed that Harness and ForgeConfig have no SandboxEnv field. Those sections were dead YAML. Sandbox env vars are delivered through host_files (.env.d/*.env files with expand: true) and GHA workflow step env: blocks, both unaffected by this change.

The test correction for code.yaml is legitimate: the old assertion expected TARGET_BRANCH which never existed in code.yaml's runner_env — it has CODE_ALLOWED_TARGET_BRANCHES, FULLSEND_OUTPUT_SCHEMA, and FULLSEND_OUTPUT_FILE. This was a pre-existing bug in the test, not a weakening.

Findings

1. Missed cleanup: .fullsend/customized/harness/code.yaml retains dead env.sandbox block — medium · incomplete-fix

File: .fullsend/customized/harness/code.yaml, lines 60–64

The PR removes env.sandbox blocks from all customized harness files it touches (fix, prioritize, retro, review, triage), but .fullsend/customized/harness/code.yaml still has:

    env:
      sandbox:
        ISSUE_NUMBER: "${ISSUE_NUMBER}"
        REPO_FULL_NAME: "${REPO_FULL_NAME}"
        GH_TOKEN: "${GH_TOKEN}"

This block is equally dead (ForgeConfig has no Env or SandboxEnv field). The scaffold version at internal/scaffold/fullsend-repo/harness/code.yaml is already clean. Leaving this creates inconsistency — future editors may copy this pattern thinking it works.

Remediation: Remove lines 60–64 from .fullsend/customized/harness/code.yaml.

2. Newly-active token references are correct and intended — low · token-activation

Files: All changed harness YAML files

Moving PUSH_TOKEN, REVIEW_TOKEN, GH_TOKEN, and PUSH_TOKEN_SOURCE from dead env.runner / forge.github.env.runner to active runner_env / forge.github.runner_env means these values will now be read, expanded, and passed to pre/post scripts at runtime. This is the intended fix — code.yaml already works this way. The ValidateRunnerEnvWith function validates that all ${VAR} references resolve before execution. RunnerEnv values stay on the runner (pre/post scripts only) and never enter the sandbox directly.

3. Git identity controls intact — low · identity-controls

File: .fullsend/customized/harness/fix.yaml, internal/scaffold/fullsend-repo/harness/fix.yaml

The removed env.sandbox in fix.yaml included GIT_AUTHOR_NAME: "fullsend-fix", GIT_AUTHOR_EMAIL: "${GIT_BOT_EMAIL}", and corresponding committer vars. These were dead (never parsed). Git identity is set through env/fix-agent.env via host_files with expand: true, which is unaffected by this change.

Dimensions reviewed

Dimension Result
Correctness ✅ Core fix is correct; one missed file
Security ✅ No privilege escalation; runner/sandbox boundary preserved
Intent & coherence ✅ Scope matches issue #349 authorization
Style & conventions ✅ Matches established code.yaml pattern
Documentation ✅ Docs already reference runner_env — no staleness
Cross-repo contracts ⏭ Skipped — no exported API changes

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

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ARCHIVED] Benchmark issue 349

0 participants