fix(workflows): make archon-adversarial-dev sed replacement macOS-safe#1155
fix(workflows): make archon-adversarial-dev sed replacement macOS-safe#1155Wirasm merged 3 commits intocoleam00:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced an in-place Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/workflows/src/defaults/bundled-defaults.test.ts (1)
122-129: Harden this regression test to cover allsed -iforms and the requiredmvstep.At Line 128, the negative assertion only blocks one exact literal. It can miss other
sed -ivariants, and the test currently doesn’t verify the atomic replace command itself.Proposed test hardening
expect(content).toContain('STATE_TMP="$ARTIFACTS/state.json.tmp"'); expect(content).toContain( 'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" > "$STATE_TMP"' ); - expect(content).not.toContain('sed -i "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/"'); + expect(content).toContain('mv "$STATE_TMP" "$ARTIFACTS/state.json"'); + expect(content).not.toMatch(/\bsed\s+-i\b/);🤖 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` around lines 122 - 129, The test for 'archon-adversarial-dev init-workspace should avoid non-portable sed -i' is too narrow; update the assertions that inspect BUNDLED_WORKFLOWS['archon-adversarial-dev'] (variable content) to assert there is no use of any sed -i form (use a negative match against a regex covering -i, -i''/"" and -i with an arg) and also assert the script uses the safe two-step atomic replace: the redirected sed invocation (existing check for 'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" > "$STATE_TMP"') plus a subsequent mv of the temp file into place (e.g., contains mv "$STATE_TMP" "$ARTIFACTS/state.json"). Ensure these checks reference the same content variable and the test name 'archon-adversarial-dev init-workspace should avoid non-portable sed -i'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/workflows/src/defaults/bundled-defaults.test.ts`:
- Around line 122-129: The test for 'archon-adversarial-dev init-workspace
should avoid non-portable sed -i' is too narrow; update the assertions that
inspect BUNDLED_WORKFLOWS['archon-adversarial-dev'] (variable content) to assert
there is no use of any sed -i form (use a negative match against a regex
covering -i, -i''/"" and -i with an arg) and also assert the script uses the
safe two-step atomic replace: the redirected sed invocation (existing check for
'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" >
"$STATE_TMP"') plus a subsequent mv of the temp file into place (e.g., contains
mv "$STATE_TMP" "$ARTIFACTS/state.json"). Ensure these checks reference the same
content variable and the test name 'archon-adversarial-dev init-workspace should
avoid non-portable sed -i'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e15a5cf-eea3-4884-bed3-b7cb77e07e60
📒 Files selected for processing (2)
.archon/workflows/defaults/archon-adversarial-dev.yamlpackages/workflows/src/defaults/bundled-defaults.test.ts
|
Thanks for this, @LaplaceYoung — the fix is correct and the regression test is well-shaped. One blocker before merge: the bundled-defaults file wasn't regenerated, and the PR's own test reads from there. Per CLAUDE.md:
Right now `packages/workflows/src/defaults/bundled-defaults.generated.ts` still contains the old `sed -i "s/..."` literal — so:
To unblockFrom the repo root, on your branch: ```bash Once that lands, `bun run validate` should pass locally and we can merge. Really appreciate the fix — this hit a real macOS pain point. |
|
I didnot take my mac mini with me recently. So I wonder would u mind me seeking help from codex in the this PR? |
|
To use Codex here, create an environment for this repo. |
1 similar comment
|
To use Codex here, create an environment for this repo. |
Sync generated bundle with the new temp-file sed pattern in archon-adversarial-dev.yaml so check:bundled passes and binary distributions ship the macOS-safe version.
|
Pushed the regenerated bundle on your behalf (also merged in current PR #1345 closed as duplicate. Should be ready to merge once CI goes green. |
2 participants
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
Chores
Tests