[widechaos]

Summary

The E2B upload list includes a normally selected .ralphrc twice: once through the project listing and again through the control-file allowlist. Keep its original position and spelling, then skip the second append. If filtering excludes the config, the allowlist still includes it once.

Fixes #389.

Validation

• Four new real-function Bats cases cover tracked, untracked, non-Git and filtered configs, including spaced nested control paths. Original source fails three cases; the patch passes all four.
• Full E2B mock suite: 108 passed. Sync-filter suite: 32 passed.
• Actual local upload with a capturing transport mock: original tar/list/sync baseline contain two config entries and log 5 files for 4 unique files; patched outputs contain one config and log 4. Checked all three listing modes and preserved order.
• Bash 3.2 syntax check and git diff --check passed.

No cloud E2B or model calls were made. This keeps the existing control-directory and sync-filter policies; it does not address the separate workspace mapping cases in #388.

[github-actions]

Review: fix(sandbox): avoid duplicate root config uploads

Clean, targeted fix for a genuine issue. Here's my assessment:

What's correct

• The dedup logic is sound. The while loop in the new pipeline stage tracks whether .ralphrc passed through sync_filter_file_list, then skips the allowlist entry if it was already emitted. Both ./ and non-./ spellings are handled by the ${f#./} strip.
• Scope is correct. Only .ralphrc needs deduplication — PROMPT.md, fix_plan.md, and AGENT.md live inside the control dir and are excluded from the generic listing. This is consistent with the issue's own analysis.
• Test coverage is solid. Four cases (tracked git, untracked git, non-git find, filtered out) exercise the three code paths: already-listed (skip allowlist), not-listed (add from allowlist), filtered (force-include once). The tar + baseline file assertions give confidence that all three affected outputs (list, archive, synced_files) are fixed.
• Order preservation is verified. The Python assertion index(.ralphrc) < index(PROMPT.md) confirms the first-position behaviour described in the comment.

Minor readability nit

The allowlist guard uses double negation:

[[ "$f" \!= .ralphrc || "$ralphrc_listed" \!= true ]] || continue

This is semantically correct (De Morgan of "skip if listed"), but the intent reads more directly written as:

[[ "$f" == .ralphrc && "$ralphrc_listed" == true ]] && continue

Not a blocker — just worth noting in case of future readers.

Test observation

The _upload_config_case helper calls git init -q . in the current directory, which is $TEST_DIR (the existing setup() does cd "$TEST_DIR"). The teardown() does cd / and then rm -rf "$TEST_DIR", so there's no cross-test pollution. The find case correctly guards against accidentally running inside a git repo with the \! git rev-parse --git-dir assertion.

The last test (filtered root config...) sources sandbox_e2b.sh directly in a subshell via run bash -c. This tests the function independently of the setup() sourcing, which is a valid approach but does source the full file each time. Since RALPH_DIR and SYNC_EXCLUDE are exported before the run, they propagate correctly.

Summary

The fix is correct, the tests are meaningful, and the blast radius is appropriately narrow. The double-negation condition is the only thing worth reconsidering for clarity, but it's not a correctness issue.

Approval: ready to merge pending that minor readability consideration (or leaving as-is — it's defensible either way).

[github-actions]

Follow-up review (second commit: test(sandbox): use an explicit non-Git fixture guard)

The second commit is a targeted improvement to the test fixture. In the initial commit, the non-git guard in the find branch was a single-line assertion. The updated form:

if git rev-parse --git-dir >/dev/null 2>&1; then
    fail "find fixture unexpectedly inside git"
fi

This is strictly better: it produces a named failure message through bats' fail rather than leaving bats to interpret a non-zero assertion exit code, which makes CI output immediately actionable. Good call.

Outstanding nit from the prior review

The double-negation guard in sandbox_e2b.sh is unchanged:

[[ "$f" \!= .ralphrc || "$ralphrc_listed" \!= true ]] || continue

As noted before, the semantically equivalent positive form is easier to parse at a glance:

[[ "$f" == .ralphrc && "$ralphrc_listed" == true ]] && continue

This is still a readability nit, not a correctness issue — both forms are logically identical by De Morgan. If the author prefers the current phrasing, it is defensible as-is.

Overall

The fix is correct, the tests are meaningful, and the second commit improves the test's diagnostic clarity. Ready to merge.