fix(workflows): make archon-adversarial-dev sed replacement macOS-safe based on #1155#1345
fix(workflows): make archon-adversarial-dev sed replacement macOS-safe based on #1155#1345LaplaceYoung wants to merge 2 commits intocoleam00:devfrom
Conversation
…mment fix(workflows): make archon-adversarial-dev sed usage portable and add regression test
📝 WalkthroughWalkthroughThis PR replaces an in-place sed operation ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workflows/src/defaults/bundled-defaults.test.ts (1)
129-129: Broaden the guard so in-place sed cannot slip in behind other flags.Line 129 only catches
sed -iwhen-iimmediately followssed;sed -E -i ...orsed --in-place ...would still pass.🧪 Proposed test hardening
- expect(content).not.toMatch(/\bsed\s+-i(?:\s*(?:''|\"\"|\S+))?\b/); + expect(content).not.toMatch(/\bsed\b[^\n]*\s(?:-i\S*|--in-place(?:=\S*)?)(?:\s|$)/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/defaults/bundled-defaults.test.ts` at line 129, The current test assertion using expect(content).not.toMatch(...) only catches sed when -i directly follows sed; update the regex in the expect(content).not.toMatch call so it matches sed invocations that include -i or --in-place anywhere in the argument list (e.g. "sed -E -i", "sed --in-place", or "sed -i -E"); specifically, broaden the pattern to look for \b sed \b followed later on the same command line by either \b-i\b or \b--in-place\b (allowing intervening flags/whitespace), then keep the existing in-place-argument handling for ''/""/file arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/workflows/defaults/archon-adversarial-dev.yaml:
- Around line 104-106: The sed->mv temp-file replacement can fail silently;
update the block that creates STATE_TMP and moves it to ARTIFACTS/state.json to
check errors: run sed and test its exit status before proceeding, verify the
temp file exists and is non-empty, and only then mv (or use an atomic rename)
while checking mv's exit status; on any failure, remove the temp file, emit a
non-zero exit via exit 1 and log an error. Specifically modify the section that
defines STATE_TMP and invokes sed/mv to add explicit checks for the sed return
code, file existence/size for STATE_TMP, and the mv return code so success echo
is only reached when all steps succeed.
---
Nitpick comments:
In `@packages/workflows/src/defaults/bundled-defaults.test.ts`:
- Line 129: The current test assertion using expect(content).not.toMatch(...)
only catches sed when -i directly follows sed; update the regex in the
expect(content).not.toMatch call so it matches sed invocations that include -i
or --in-place anywhere in the argument list (e.g. "sed -E -i", "sed --in-place",
or "sed -i -E"); specifically, broaden the pattern to look for \b sed \b
followed later on the same command line by either \b-i\b or \b--in-place\b
(allowing intervening flags/whitespace), then keep the existing
in-place-argument handling for ''/""/file arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a0bfb0b-967d-4124-90ec-1e5e3e642f33
📒 Files selected for processing (2)
.archon/workflows/defaults/archon-adversarial-dev.yamlpackages/workflows/src/defaults/bundled-defaults.test.ts
|
Closing as duplicate of #1155 — the original PR from @LaplaceYoung. I merged |
Summary
Fixes the
archon-adversarial-devinit-workspace step for macOS BSDsedcompatibility.Issue #1103 reports that
sed -ifails on macOS because BSD sed requires a backup extension, while GNU sed accepts bare-i.This PR replaces the non-portable in-place edit with a portable temp-file pattern:
sed -i s/.../.../ filesed s/.../.../ file > file.tmp && mv file.tmp fileChanges
.archon/workflows/defaults/archon-adversarial-dev.yamlinit-workspaceto writestate.jsonvia temp file (state.json.tmp) and atomic move.packages/workflows/src/defaults/bundled-defaults.test.tssed -ifor this replacement.Validation
bun test packages/workflows/src/defaults/bundled-defaults.test.tsCloses #1103
Summary by CodeRabbit