test(macos): stop conductor tests depending on what the build staged - #371
Merged
Conversation
Six tests asserted that no `burrow` conductor was bundled, without ever saying so. Resources/burrow only appears when the vendor/burrow-cli submodule is checked out -- which a developer must do for the Network, Orphans and Photos panes to work at all -- so the suite was green on CI, where actions/checkout fetches no submodules, and red on a correctly-configured checkout, for reasons unrelated to the code under test. BurrowConductor now resolves its sidecars through an injectable `resourceDirectory` instead of reading Bundle.main directly, and ConductorBundleFixture stages a temporary directory that either is empty or holds an executable stub. Each test now declares which build it exercises. Verified green both with the conductor bundled and with it removed. Two of those tests were not testing what they claimed. Streaming is ON by default -- `streamingEnabled` returns true when the switch is unset, as its own comment says -- so `testStreamOverride_offByDefault_keepsDirectEngine` passed only because streamOverride bailed at its last guard, the executable lookup. It asserted nothing about the switch and would have kept passing had the default flipped either way. It is now three tests: no conductor falls back, a bundled conductor routes through by default, and the documented kill-switch actually kills it. testMissingExecutableFailsBeforeSpawn had the same shape -- with a conductor staged, streamOverride supplies an executable before resolveMo is consulted, so the branch under test was unreachable. Adds the conductor-present coverage that did not exist: bad arguments still surface as badArguments rather than a conductor error, and a non-executable file named `burrow` does not count as one.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds injectable bundled-resource lookup for ChangesConductor resource resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macos/Tests/BurrowEnvelopeTests.swift`:
- Around line 158-163: Update the affected tests around
BurrowConductor.streamOverride to capture UserDefaults.standard.object(forKey:
"BurrowStreamViaConductor") before mutation and restore it in a defer block. Set
the saved value when it existed, and remove the key only when it was originally
absent, including the additional test ranges noted.
In `@macos/Tests/MCPConductorToolsTests.swift`:
- Around line 202-210: Update
testSlimCheck_withConductorBundled_stillRejectsBadArgumentsWithoutSpawning to
assert that catalog.call throws MCPToolError.badArguments directly, including
its associated missing-argument value, instead of converting the error to a
string and checking substrings; retain the
validation-before-conductor-availability expectation.
- Around line 220-222: Assert that FileManager.default.createFile in the
non-executable burrow fixture returns true, so the test fails if artifact
creation does not succeed before evaluating executableURL().
🪄 Autofix
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 Plus
Run ID: 65ef78d4-010b-4377-a9d7-292baab7a60f
📒 Files selected for processing (5)
macos/Sources/BurrowConductor.swiftmacos/Tests/BurrowEnvelopeTests.swiftmacos/Tests/ConductorBundleFixture.swiftmacos/Tests/MCPConductorToolsTests.swiftmacos/Tests/OperationFlowTests.swift
Comment on lines
+158
to
+163
| UserDefaults.standard.removeObject(forKey: "BurrowStreamViaConductor") | ||
| ConductorBundleFixture.withConductor(present: true) { | ||
| let override = BurrowConductor.streamOverride(moArgs: ["clean"], elevated: false) | ||
| XCTAssertEqual(override?.arguments, ["clean", "--apply", "--stream"]) | ||
| XCTAssertEqual(URL(fileURLWithPath: override?.executable ?? "").lastPathComponent, "burrow") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the prior BurrowStreamViaConductor value.
These tests overwrite or remove the shared default. Each test then removes the key instead of restoring its original value. Save object(forKey:) before the mutation and restore that object in defer; remove the key only when it was originally absent.
Also applies to: 168-172, 176-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macos/Tests/BurrowEnvelopeTests.swift` around lines 158 - 163, Update the
affected tests around BurrowConductor.streamOverride to capture
UserDefaults.standard.object(forKey: "BurrowStreamViaConductor") before mutation
and restore it in a defer block. Set the saved value when it existed, and remove
the key only when it was originally absent, including the additional test ranges
noted.
Comment on lines
+202
to
+210
| func testSlimCheck_withConductorBundled_stillRejectsBadArgumentsWithoutSpawning() throws { | ||
| try ConductorBundleFixture.withConductor(present: true) { | ||
| XCTAssertThrowsError(try catalog.call(name: "burrow_slim_check", arguments: [:])) { error in | ||
| let described = String(describing: error) | ||
| XCTAssertTrue(described.contains("binary"), | ||
| "the caller must be told which argument is missing, got: \(described)") | ||
| XCTAssertFalse(described.localizedCaseInsensitiveContains("conductor"), | ||
| "a missing argument must not be reported as a missing conductor") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert MCPToolError.badArguments directly.
String(describing:) can accept another error that happens to contain binary. Match MCPToolError.badArguments so this test proves that validation wins before conductor availability.
Proposed fix
try ConductorBundleFixture.withConductor(present: true) {
XCTAssertThrowsError(try catalog.call(name: "burrow_slim_check", arguments: [:])) { error in
- let described = String(describing: error)
- XCTAssertTrue(described.contains("binary"),
- "the caller must be told which argument is missing, got: \(described)")
- XCTAssertFalse(described.localizedCaseInsensitiveContains("conductor"),
- "a missing argument must not be reported as a missing conductor")
+ guard case MCPToolError.badArguments = error else {
+ return XCTFail("expected .badArguments, got \(error)")
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func testSlimCheck_withConductorBundled_stillRejectsBadArgumentsWithoutSpawning() throws { | |
| try ConductorBundleFixture.withConductor(present: true) { | |
| XCTAssertThrowsError(try catalog.call(name: "burrow_slim_check", arguments: [:])) { error in | |
| let described = String(describing: error) | |
| XCTAssertTrue(described.contains("binary"), | |
| "the caller must be told which argument is missing, got: \(described)") | |
| XCTAssertFalse(described.localizedCaseInsensitiveContains("conductor"), | |
| "a missing argument must not be reported as a missing conductor") | |
| } | |
| func testSlimCheck_withConductorBundled_stillRejectsBadArgumentsWithoutSpawning() throws { | |
| try ConductorBundleFixture.withConductor(present: true) { | |
| XCTAssertThrowsError(try catalog.call(name: "burrow_slim_check", arguments: [:])) { error in | |
| guard case MCPToolError.badArguments = error else { | |
| return XCTFail("expected .badArguments, got \(error)") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macos/Tests/MCPConductorToolsTests.swift` around lines 202 - 210, Update
testSlimCheck_withConductorBundled_stillRejectsBadArgumentsWithoutSpawning to
assert that catalog.call throws MCPToolError.badArguments directly, including
its associated missing-argument value, instead of converting the error to a
string and checking substrings; retain the
validation-before-conductor-availability expectation.
Comment on lines
+220
to
+222
| FileManager.default.createFile(atPath: dir.appendingPathComponent("burrow").path, | ||
| contents: Data("not a binary".utf8), | ||
| attributes: [.posixPermissions: 0o644]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline macos/Tests/MCPConductorToolsTests.swift
printf '%s\n' '--- target lines ---'
sed -n '180,250p' macos/Tests/MCPConductorToolsTests.swift
printf '%s\n' '--- related symbols ---'
rg -n -C 4 'executableURL|createFile\\(' macos/Tests macos --glob '*.swift' | head -n 240Repository: caezium/Burrow
Length of output: 4304
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- BurrowConductor definitions and references ---'
rg -n -C 6 'executableURL|resourceDirectory|isAvailable|struct BurrowConductor|enum BurrowConductor|class BurrowConductor' macos --glob '*.swift'
printf '%s\n' '--- createFile usage ---'
rg -n -C 4 'createFile' macos --glob '*.swift'
printf '%s\n' '--- fixture definitions ---'
rg -n -C 8 'ConductorBundleFixture' macos --glob '*.swift'Repository: caezium/Burrow
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
test = Path("macos/Tests/MCPConductorToolsTests.swift").read_text()
source = Path("macos/Sources/BurrowConductor.swift").read_text()
call = re.search(
r'(?m)^\s*FileManager\.default\.createFile\(\s*'
r'atPath:\s*dir\.appendingPathComponent\("burrow"\)\.path,'
r'.*?\n\s*attributes:\s*\[\.posixPermissions:\s*0o644\]\)',
test,
re.S,
)
assert call, "target createFile call not found"
call_text = call.group(0)
assert not re.search(r'=\s*FileManager\.default\.createFile', call_text), \
"target createFile result is already captured"
lookup = re.search(
r'(?m)^\s*let burrow = res\.appendingPathComponent\("burrow"\)\s*\n'
r'\s*return FileManager\.default\.isExecutableFile\(atPath:\s*burrow\.path\)\s*\?\s*burrow\s*:\s*nil',
source,
)
assert lookup, "executableURL does not gate on isExecutableFile"
assert "XCTAssertNil(BurrowConductor.executableURL())" in test
assert "XCTAssertFalse(BurrowConductor.isAvailable)" in test
print("createFile result is ignored")
print("executableURL returns nil when burrow is absent or non-executable")
print("the test's current assertions cannot distinguish those two states")
PYRepository: caezium/Burrow
Length of output: 313
Assert that the non-executable artifact was created.
If createFile fails, executableURL() remains nil, so the test can pass without testing the non-executable burrow file. Assert that createFile returns true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macos/Tests/MCPConductorToolsTests.swift` around lines 220 - 222, Assert that
FileManager.default.createFile in the non-executable burrow fixture returns
true, so the test fails if artifact creation does not succeed before evaluating
executableURL().
The streaming-switch tests wrote and then REMOVED `BurrowStreamViaConductor`, and the suite runs against UserDefaults for the real app domain — so running the tests erased a kill-switch a developer had genuinely set. They now save the prior value, absent or not, and restore it. Two assertions also proved less than they looked: the bad-arguments check matched substrings of `String(describing:)` rather than the error, so a different error carrying the same words would have passed, and the non-executable fixture never checked that its file was actually created.
caezium
added a commit
that referenced
this pull request
Aug 10, 2026
…371) * test(macos): stop conductor tests depending on what the build staged Six tests asserted that no `burrow` conductor was bundled, without ever saying so. Resources/burrow only appears when the vendor/burrow-cli submodule is checked out -- which a developer must do for the Network, Orphans and Photos panes to work at all -- so the suite was green on CI, where actions/checkout fetches no submodules, and red on a correctly-configured checkout, for reasons unrelated to the code under test. BurrowConductor now resolves its sidecars through an injectable `resourceDirectory` instead of reading Bundle.main directly, and ConductorBundleFixture stages a temporary directory that either is empty or holds an executable stub. Each test now declares which build it exercises. Verified green both with the conductor bundled and with it removed. Two of those tests were not testing what they claimed. Streaming is ON by default -- `streamingEnabled` returns true when the switch is unset, as its own comment says -- so `testStreamOverride_offByDefault_keepsDirectEngine` passed only because streamOverride bailed at its last guard, the executable lookup. It asserted nothing about the switch and would have kept passing had the default flipped either way. It is now three tests: no conductor falls back, a bundled conductor routes through by default, and the documented kill-switch actually kills it. testMissingExecutableFailsBeforeSpawn had the same shape -- with a conductor staged, streamOverride supplies an executable before resolveMo is consulted, so the branch under test was unreachable. Adds the conductor-present coverage that did not exist: bad arguments still surface as badArguments rather than a conductor error, and a non-executable file named `burrow` does not count as one. * test(macos): stop the conductor tests editing real preferences The streaming-switch tests wrote and then REMOVED `BurrowStreamViaConductor`, and the suite runs against UserDefaults for the real app domain — so running the tests erased a kill-switch a developer had genuinely set. They now save the prior value, absent or not, and restore it. Two assertions also proved less than they looked: the bad-arguments check matched substrings of `String(describing:)` rather than the error, so a different error carrying the same words would have passed, and the non-executable fixture never checked that its file was actually created.
caezium
added a commit
that referenced
this pull request
Aug 10, 2026
* fix: complete safe release hardening * fix: bind helper operations to invoking user * docs: record 0.12 release verification * fix: close safe release review gaps * fix(macos): report what the cleanup actually did Reverts the manifest-based irreversible cleanup added in 9f3208f and restores `find -x <path> -depth -delete` behind the existing boundary checks. The manifest walked every reviewed tree in the GUI to enumerate descendants, then spawned one root process per node; on a large Xcode cache that is tens of thousands of spawns, and any entry whose volume differed from the quarantine root aborted the whole run. The deletes now live next to the plan that authorizes them, as `CleanupExecutionPlan.irreversibleCleanupShell()`, so a test can execute the real shell rather than a reimplementation of it. Two reporting bugs went with it: * `find -delete` is documented to always return true. It exits 0 having printed "Permission denied" and removed nothing, so its exit status was never a success signal -- a clean that freed nothing reported success. Each entry is now re-checked after the delete and the shell fails on any survivor. * One failing entry aborted the remaining entries and the run was reported as if nothing had happened. Entries are independent now, and `failureMessage` distinguishes "nothing ran" from "ran and partly failed" instead of telling users to rescan after a partial delete. Wrapper exit codes are named in `ElevatedExitCode` rather than bare 124/125/126/127 spread across two files, and a refusal before the auth prompt now carries its reason into the transcript instead of surfacing as an unexplained 126. * fix(macos): stop refusing the engine on every real installation `ValidatedElevatedCommand.prepare` required root ownership of the privileged executable and of every ancestor directory, with no group-writable component anywhere. No shipped copy of Burrow can satisfy that. `/Applications` is root:admin mode 0775 on stock macOS, and an app dragged there -- or installed by the Homebrew cask -- is owned by the account that installed it. Optimize therefore refused before raising a prompt on any install that wasn't running from a root-owned build directory, which is every install except a developer's. Ownership is now a policy rather than one rule. System tools outside the bundle keep the strict version. Inside our own signed bundle, ownership may be root or the invoking user and only world-writable components are refused -- group-writable is how macOS ships /Applications, so refusing it refuses the platform. What pays for the relaxation is the resource seal: `codesign --verify --strict` already runs as root at the execution boundary, and every ancestor stays pinned and re-checked there regardless of who owns it. The tests build the real /Applications layout -- group-writable parent, user-owned bundle -- which is the coverage whose absence let a rule that cannot hold in production ship as a hardening change. * feat(macos): restore Touch ID for the reviewed clean Clean was the one elevated operation that never reached the privileged helper. Routing recognises engine argv, and once the permanent clean became a reviewed plan rather than a `mo clean` invocation it had no argv to recognise, so it fell through to osascript -- which authenticates through SecurityAgent's classic mechanism and is password-only by construction. The most consequential operation Burrow performs quietly lost Touch ID while every other one kept it. It cannot simply route to the existing `clean` operation: that runs the engine's own selection rules and discards the entries the user reviewed. So this adds `HelperOperation.cleanReviewed`, the only operation that carries data from the caller beyond a verb. That does not weaken the property the helper rests on. argv is still composed daemon-side, the executable is still a fixed absolute path (/usr/bin/find joins the closed system-tool set), and the paths are operands to a delete rather than anything executed. What makes it safe is that the daemon does not trust the list: it rebuilds the approved roots from its own getpwuid record and re-derives every fact with its own lstat. An entry must exist, not be a symlink, equal its own canonical path -- so no ancestor is a link either -- sit strictly inside the invoking user's home or a system cache, be on that root's volume, and outside the shared system caches belong to the invoking user. That last rule is the one that matters: /private/var/folders holds other accounts' trees, and root deleting those is an escalation the user could not perform themselves. The list is capped and one bad entry refuses the whole request. Success is the postcondition, not find's exit status, for the same reason as on the osascript path. Two daemon fixes in the same files: * Cancellation is bound to the invoking uid. On a Mac with several signed-in users, knowing an operation ID was enough to stop another session's root work. * The output splitter buffers bytes rather than strings. A read ending mid-UTF-8-sequence made `String(data:encoding:)` return nil and the whole chunk was dropped, losing entire lines of a root operation's output -- and paths are exactly where non-ASCII shows up. * fix: close the remaining safe-release gaps Replay guard: eviction was a count-bounded FIFO, so a caller could send `capacity` fresh operation IDs to push an older one out of the set and then replay that older payload. The bound meant to prevent replay was itself the bypass. Eviction is by age now -- an ID is only forgotten once it is far older than any authorization could still be valid for -- with the count cap kept as a backstop that only discards already-expired entries. Update verification: `SecStaticCodeCheckValidity` ran with a nil requirement, which validates against the code's own designated requirement -- something a self-signed bundle satisfies trivially. The only other thing pinned was the team identifier, a string in the certificate subject that a self-signed certificate is free to claim. It now checks `anchor apple generic`, which only Apple-issued chains satisfy, and a requirement that fails to build refuses rather than skipping the check. Rate limiting (macOS and Windows): the limiter ran only after a request had authenticated, leaving rejected requests uncounted. That gave an attacker unlimited token guesses and unlimited parse work from the same loopback socket -- the exact load the limiter exists to cap. Both count before authorization now. Elevated log tail: the poll ran on the main run loop, so a busy or modal UI could miss the short window the root shell allows before its trap unlinks the sink, losing the entire transcript. It runs on the stream queue now, which also puts every access to the log handle on the one queue that owns it -- previously the timer and the termination handler raced to open, read and close it, including reads against a descriptor the other side had already closed. The GUI's decode sites use lossy decoding so a chunk boundary mid-sequence can no longer drop output. Also corrects the helper code-requirement comment, which claimed no team identifier appears anywhere in the repository; the release workflow pins one as a build-time assertion. The runtime check still discovers it. * fix(macos): stop reporting a successful clean as exit 1 The engine's export list names a parent AND its own children as separate entries. The helper deleted them in plan order, so a parent went first and every nested entry under it was already gone when its turn came -- `find` exited nonzero with "No such file or directory" for work that had actually succeeded, and the run surfaced as a failure. Two causes, both fixed. The helper path used raw plan order while the osascript path had always sorted deepest-first; both now share `orderedReviewedPaths()`, so no entry can vanish before it is reached. And the daemon treated `find`'s status as the verdict despite its own comment saying the postcondition is authoritative -- `-delete` returns true when it removed nothing and false for an entry a deeper delete already took, so it was never a usable signal. For reviewed cleans the daemon now decides purely on whether each path is gone afterwards, in both directions: survivors fail the run, and an absent entry is a success whatever `find` said on its way out. Entries the snapshot refuses now carry a `notCleanable` lock reason and render a "Can't clean" chip with the reason on hover, rather than sitting tickable in a list that could never act on them. They stay listed because the scan preview counted them, and silently dropping a line the user was just shown reads as a miscount. Clean now sits beside Scan your Mac on the idle hero for anyone who already trusts the engine's selection. It routes to the plain `clean` operation -- no plan, so no review -- and the elevation prompt is still ahead of any deletion. * fix: address review findings across the safe-release surface Verified each finding against the code rather than applying it; two were rejected on evidence and are left alone, with the reasons recorded below. Genuine defects: - The reviewed-clean header counted every locked entry but collected only .appOpen names, so a run whose sole locked entry was one the snapshot refused rendered "Close to clean another 12 GB" -- an empty app name promising that quitting something frees bytes no app holds. Restricted to .appOpen, with the neutral message when nothing is app-held. - HelperService shared ONE HelperLineSplitter between the stdout and stderr reader threads. That buffer exists to carry a partial line, so sharing it was both a data race and a splice: half a stdout line could be prefixed onto the next stderr chunk, emitting lines that appeared on neither stream. One splitter per pipe, each flushed separately. - HelperReplayGuard grew without bound. Age-based eviction is right -- a count-bounded FIFO lets a flood push out an ID that is still replayable -- but "keep everything inside the retention window" has no ceiling in a process running as root. It now refuses new IDs at a hard ceiling instead of forgetting old ones: a refused request is recoverable, a forgotten ID is replayable. - Cancelling a Sparkle update never resumed its checked continuation. Cancelling a Task does not resume one, so the session stayed registered and every `sparkleSessions.isEmpty` gate stayed shut for the rest of the launch. Cancel now settles through the existing idempotent finishOnce. - OperationFlow closed its pipe write copies only on the unelevated path, though the elevated branch hands the same two pipes to osascript and ends with readDataToEndOfFile on each -- a read that returns only once every write descriptor is gone, this parent's included. - The elevation failure named Homebrew as the trusted location, which trustedExecutable() deliberately stopped honouring; it pointed users at the one place that could never satisfy it. - safeDiagnosticLabel matched `token=` only when written tight, so `access_token = ...` and `api key=...` carried credentials into an uploaded diagnostic. Now matched by credential-name-plus-separator. - The stdio bridge took BURROWWIN_MCP_ENDPOINT unvalidated and then attached the stored bearer token to every request sent there, so any process able to set that variable redirected the bridge AND handed over the local MCP credential. Non-loopback endpoints now fall back. - Only Sec-Fetch-Site was rejected, so a request carrying just Sec-Fetch-Mode or Sec-Fetch-Dest reached the loopback server. Any fetch-metadata header now forbids. - No size ceiling existed on an update archive before it was kept and handed to ditto, which is where a small archive expands without bound. Correctness of the record, and tests that proved nothing: - SECURITY.md said four permitted Apple tools; the helper permits five. - macos-signing.md omitted SENTRY_AUTH_TOKEN, which sentry-issues.yml requires and which fails by warning rather than erroring when absent. - The staging-directory test asserted the name did not contain the literal source text "UUID().uuidString", which no path could contain, so it passed regardless. It now checks the real mkdtemp shape. - testSteps_neverInvokeAShell passed no reviewed paths, so the one step built from caller-supplied data was never examined. - The query-server request helper duplicated the token literal, and the cleanup test failed as root, where the permission denial it needs cannot happen. - TuneUp's notices and the query-server auth value were never localizable; zh-Hans already carried a translation that could not be reached. - Pressing Run with a stale cleanup plan returned silently. It now says so, and refuses only the clean so maintenance still runs. Rejected, with the evidence: - Making /Library/Logs allowsForeignOwner: false would disable the feature. That tree is root:wheel with 38 root-owned and 2 _windowserver-owned entries and none owned by the invoking user, so requiring an owner match refuses every entry. Foreign ownership is exactly why it needs true; the escalation concern applies to per-user trees, which are already false. - Admitting the replay guard before identity resolution would let anyone flood it with unvalidated IDs -- the surface the new ceiling closes -- and a full guard then fails closed on the legitimate user. Today a caller must pass the same-user identity check before it can burn an ID. * test(macos): stop conductor tests depending on what the build staged (#371) * test(macos): stop conductor tests depending on what the build staged Six tests asserted that no `burrow` conductor was bundled, without ever saying so. Resources/burrow only appears when the vendor/burrow-cli submodule is checked out -- which a developer must do for the Network, Orphans and Photos panes to work at all -- so the suite was green on CI, where actions/checkout fetches no submodules, and red on a correctly-configured checkout, for reasons unrelated to the code under test. BurrowConductor now resolves its sidecars through an injectable `resourceDirectory` instead of reading Bundle.main directly, and ConductorBundleFixture stages a temporary directory that either is empty or holds an executable stub. Each test now declares which build it exercises. Verified green both with the conductor bundled and with it removed. Two of those tests were not testing what they claimed. Streaming is ON by default -- `streamingEnabled` returns true when the switch is unset, as its own comment says -- so `testStreamOverride_offByDefault_keepsDirectEngine` passed only because streamOverride bailed at its last guard, the executable lookup. It asserted nothing about the switch and would have kept passing had the default flipped either way. It is now three tests: no conductor falls back, a bundled conductor routes through by default, and the documented kill-switch actually kills it. testMissingExecutableFailsBeforeSpawn had the same shape -- with a conductor staged, streamOverride supplies an executable before resolveMo is consulted, so the branch under test was unreachable. Adds the conductor-present coverage that did not exist: bad arguments still surface as badArguments rather than a conductor error, and a non-executable file named `burrow` does not count as one. * test(macos): stop the conductor tests editing real preferences The streaming-switch tests wrote and then REMOVED `BurrowStreamViaConductor`, and the suite runs against UserDefaults for the real app domain — so running the tests erased a kill-switch a developer had genuinely set. They now save the prior value, absent or not, and restore it. Two assertions also proved less than they looked: the bad-arguments check matched substrings of `String(describing:)` rather than the error, so a different error carrying the same words would have passed, and the non-executable fixture never checked that its file was actually created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #367 — review that first; this branch's diff is only the last commit.
The problem
Six tests asserted that no
burrowconductor was bundled, without ever saying so.Resources/burrowonly appears when thevendor/burrow-clisubmodule is checked out — which you must do for the Network, Orphans and Photos panes to work at all. So the suite was green on CI, whereactions/checkoutfetches no submodules, and red on a correctly-configured checkout, for reasons unrelated to the code under test.The fix
BurrowConductorresolves its sidecars through an injectableresourceDirectoryinstead of readingBundle.maindirectly, andConductorBundleFixturestages a temp directory that is either empty or holds an executable stub. Each test now declares which build it exercises.Verified green both ways — 1013 tests, 0 failures with the conductor bundled, and again with it removed.
Two tests were not testing what they claimed
testStreamOverride_offByDefault_keepsDirectEngineasserted streaming is off by default. It isn't —streamingEnabledreturnstruewhen the switch is unset, as its own doc comment says. The test passed only becausestreamOverridebailed at its last guard, the executable lookup, so it asserted nothing about the switch and would have kept passing had the default flipped either way. It's now three tests: no conductor falls back, a bundled conductor routes through by default, and the documenteddefaults write … BurrowStreamViaConductor -bool NOkill-switch actually kills it.testMissingExecutableFailsBeforeSpawnhad the same shape — with a conductor staged,streamOverridesupplies an executable beforeresolveMois consulted, so the unresolvable-engine branch it exists to check was unreachable.Also adds
The conductor-present coverage that didn't exist: bad arguments still surface as
badArgumentsrather than a conductor error, and a non-executable file namedburrowdoesn't count as one.Summary by CodeRabbit