Tests - #4
Conversation
📝 WalkthroughWalkthroughAdds extensive unit tests and test infrastructure for workspace management, introduces environment-driven persistence isolation and test-mode logging control, adds an isolated build/run workflow in the Makefile, updates .gitignore, and registers a new test target in Package.swift. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
Makefile (1)
1-1: Add a top-leveltesttarget.This PR adds a real
ShellraiserTestssuite, but the Makefile still exposes only build/run entry points. A phonytesttarget would make the new coverage part of the normal workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add a top-level phony test target by updating the .PHONY line to include "test" and adding a new "test:" rule that invokes the project’s test suite (the newly added ShellraiserTests) and any coverage/reporting commands you want run as part of CI (e.g., call the existing test runner script or run the test command used elsewhere in the repo). Ensure the new target name is "test" and that it appears alongside the existing phony targets "build-app", "run", "build-isolated", and "run-isolated" so running `make test` executes the ShellraiserTests and generates coverage.Tests/ShellraiserTests/WorkspaceManagerCompletionTests.swift (1)
136-144: Consider using direct enum assertions for clearer test intent.The pattern
if case .current = ... { } else { XCTFail(...) }is unusual. Swift's XCTest doesn't have built-in enum case assertions, but you could extract a helper or use a more conventional assertion style.♻️ Suggested alternative
- if case .current = manager.completionHighlightState(workspaceId: workspaceId, paneId: currentPaneId) { - } else { - XCTFail("Expected current pane highlight state.") - } - - if case .queued = manager.completionHighlightState(workspaceId: workspaceId, paneId: queuedPaneId) { - } else { - XCTFail("Expected queued pane highlight state.") - } + let currentState = manager.completionHighlightState(workspaceId: workspaceId, paneId: currentPaneId) + let queuedState = manager.completionHighlightState(workspaceId: workspaceId, paneId: queuedPaneId) + XCTAssertEqual(currentState, .current, "Expected current pane highlight state.") + XCTAssertEqual(queuedState, .queued, "Expected queued pane highlight state.")This requires
CompletionPaneHighlightStateto conform toEquatable, which it likely already does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/WorkspaceManagerCompletionTests.swift` around lines 136 - 144, Replace the awkward `if case` checks with direct enum equality assertions: call manager.completionHighlightState(workspaceId: workspaceId, paneId: currentPaneId) and assert it equals CompletionPaneHighlightState.current (and similarly assert equals .queued for queuedPaneId) using XCTest assertion helpers (e.g. XCTAssertEqual); ensure CompletionPaneHighlightState conforms to Equatable if it does not already so the direct equality assertion compiles.Tests/ShellraiserTests/AgentCompletionEventMonitorTests.swift (1)
75-80: Consider using XCTUnwrap or do-catch for better test failure diagnostics.The
try!force-unwraps in the helper could cause test crashes instead of clear assertion failures if file operations fail unexpectedly. UsingXCTUnwrapor a throwing helper propagates failures more gracefully.iology♻️ Suggested refactor
- private func appendLine(_ line: String, to url: URL) { - let handle = try! FileHandle(forWritingTo: url) - try! handle.seekToEnd() - handle.write(Data((line + "\n").utf8)) - try! handle.close() + private func appendLine(_ line: String, to url: URL) throws { + let handle = try FileHandle(forWritingTo: url) + try handle.seekToEnd() + handle.write(Data((line + "\n").utf8)) + try handle.close() }Then update call sites to use
try:try appendLine("...", to: logURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/AgentCompletionEventMonitorTests.swift` around lines 75 - 80, The helper appendLine(_ line: String, to url: URL) currently uses try! which will crash tests on IO failures; change appendLine to be a throwing function (remove force-unwraps and use try/throw or use do-catch to rethrow a descriptive error) or return a Bool and call XCTUnwrap where appropriate, and update all call sites to call try appendLine(...) so failures surface as test failures instead of crashes; look for the appendLine symbol and replace its FileHandle(forWritingTo:), seekToEnd(), write(...), and close() try! usages with proper try/error propagation or XCTUnwrap handling.Tests/ShellraiserTests/PaneNodeModelOperationsTests.swift (1)
244-275: Consider extending WorkspaceTestCase to reuse shared test fixtures.This test class defines its own
makeSurfaceandmakeLeafhelpers, which appear similar to those inWorkspaceTestSupport.swift. If the helpers are compatible, extendingWorkspaceTestCasewould reduce duplication.If the simpler signatures here are intentional (e.g., fewer parameters for focused pane-tree tests), keeping them local is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/PaneNodeModelOperationsTests.swift` around lines 244 - 275, Replace the duplicated helper implementations by making this test class inherit from WorkspaceTestCase and reuse the shared fixture helpers from WorkspaceTestSupport; remove the local makeSurface(id:title:) and makeLeaf(paneId:surfaceId:) implementations and either call the existing shared makeSurface/makeLeaf helpers directly or add tiny forwarding wrappers that adapt their simpler signatures to the shared helper signatures (referencing the makeSurface and makeLeaf symbols) so the tests use the centralized fixtures instead of duplicating them.Tests/ShellraiserTests/WorkspaceTestSupport.swift (2)
126-126: Add a brief comment explaining theNSApplication.sharedinvocation.The
_ = NSApplication.sharedpattern ensures AppKit initialization but isn't self-documenting. A brief comment would help future maintainers understand its purpose.📝 Suggested improvement
) -> WorkspaceManager { - _ = NSApplication.shared + // Ensure NSApplication is initialized before creating UI-dependent managers + _ = NSApplication.shared return WorkspaceManager(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/WorkspaceTestSupport.swift` at line 126, Add a brief inline comment on the line that calls NSApplication.shared (the _ = NSApplication.shared invocation) explaining that this forces AppKit to initialize (loads the shared NSApplication instance and sets up AppKit environment for tests that interact with UI components) so future maintainers understand why the no-op assignment is required.
14-34: Consider thread-safety for parallel test execution.Using
setenv/unsetenvis not thread-safe. If XCTest runs tests in parallel (e.g., across multiple test classes), concurrent environment variable modifications could cause flaky failures. The current approach works for serial execution within the same test class.If parallel testing is needed in the future, consider injecting the subdirectory path directly into
WorkspacePersistencevia an initializer parameter rather than relying on environment variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/WorkspaceTestSupport.swift` around lines 14 - 34, The test helper makePersistenceContext currently uses setenv/unsetenv which is not thread-safe; update WorkspacePersistence to accept an injected appSupportSubdirectory (or a full appSupport URL) via a new initializer or factory (e.g., add init(appSupportSubdirectory: String) or init(appSupportURL: URL) to WorkspacePersistence), then change makePersistenceContext to construct the subdirectory and pass it directly to WorkspacePersistence instead of calling setenv/unsetenv; remove the environment manipulation and teardown that unsets those keys and instead keep only the filesystem cleanup of the created directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Shellraiser/Services/Persistence/WorkspacePersistence.swift`:
- Around line 24-31: The appSupportSubdirectory() function currently returns the
environment override verbatim which can contain "/" or ".." and allow
workspaceFileURL to escape the intended Application Support folder; change
appSupportSubdirectory() to validate the override value: reject any override
containing "/" or "\" or path components equal to "." or ".." (or any
multi-component path), and only accept a single safe path component (e.g.,
non-empty and matches a filename-safe pattern) before returning it; if
validation fails, fall back to the original default behavior so workspaceFileURL
remains constrained.
In `@Tests/ShellraiserTests/AgentCompletionEventTests.swift`:
- Around line 20-27: Add a test asserting AgentCompletionEvent.parse rejects a
line with an invalid base64 payload (e.g., a well-formed timestamp/type/uuid
followed by a non-base64 string) so the parser fails instead of accepting an
empty payload; then update the parser implementation in
AgentCompletionEvent.parse to stop using Data(base64Encoded:) ?? Data() and
instead guard-let the result of Data(base64Encoded:) (returning nil on failure)
so corrupted payloads are rejected.
In `@Tests/ShellraiserTests/WorkspaceManagerCommandTests.swift`:
- Around line 29-31: The test reuses the same Surface instance (firstSurface) in
both makeLeaf calls for singlePaneId and multiPaneId which can hide
identity-related bugs; create a distinct Surface instance (e.g.,
secondPaneSurface) and use that for the second makeLeaf (the one building the
leaf for multiPaneId) so closeItemTitle and any model lookups use unique surface
identities; update references in the test setup where makeLeaf(paneId:
multiPaneId, surfaces: [...], activeSurfaceId: ...) is called to pass the new
surface and its id instead of firstSurface.
In `@Tests/ShellraiserTests/WorkspaceSurfaceManagerSurfaceStateTests.swift`:
- Around line 275-278: The test currently force-unwraps
WorkspacePersistence.appSupportSubdirectoryEnvironmentKey when building the URL
for removal which will crash if the env var is missing; change this to safely
unwrap (using guard let or XCTUnwrap) and fail the test with a clear message if
the environment value is absent, or better yet obtain the directory directly
from makePersistenceContext() (or the persistence context API) and use that
directory for FileManager.default.removeItem to avoid relying on the environment
variable; update any references in this test method to use the safe-unwrapped
value or the persistence context's directory instead of the forced unwrap.
---
Nitpick comments:
In `@Makefile`:
- Line 1: Add a top-level phony test target by updating the .PHONY line to
include "test" and adding a new "test:" rule that invokes the project’s test
suite (the newly added ShellraiserTests) and any coverage/reporting commands you
want run as part of CI (e.g., call the existing test runner script or run the
test command used elsewhere in the repo). Ensure the new target name is "test"
and that it appears alongside the existing phony targets "build-app", "run",
"build-isolated", and "run-isolated" so running `make test` executes the
ShellraiserTests and generates coverage.
In `@Tests/ShellraiserTests/AgentCompletionEventMonitorTests.swift`:
- Around line 75-80: The helper appendLine(_ line: String, to url: URL)
currently uses try! which will crash tests on IO failures; change appendLine to
be a throwing function (remove force-unwraps and use try/throw or use do-catch
to rethrow a descriptive error) or return a Bool and call XCTUnwrap where
appropriate, and update all call sites to call try appendLine(...) so failures
surface as test failures instead of crashes; look for the appendLine symbol and
replace its FileHandle(forWritingTo:), seekToEnd(), write(...), and close() try!
usages with proper try/error propagation or XCTUnwrap handling.
In `@Tests/ShellraiserTests/PaneNodeModelOperationsTests.swift`:
- Around line 244-275: Replace the duplicated helper implementations by making
this test class inherit from WorkspaceTestCase and reuse the shared fixture
helpers from WorkspaceTestSupport; remove the local makeSurface(id:title:) and
makeLeaf(paneId:surfaceId:) implementations and either call the existing shared
makeSurface/makeLeaf helpers directly or add tiny forwarding wrappers that adapt
their simpler signatures to the shared helper signatures (referencing the
makeSurface and makeLeaf symbols) so the tests use the centralized fixtures
instead of duplicating them.
In `@Tests/ShellraiserTests/WorkspaceManagerCompletionTests.swift`:
- Around line 136-144: Replace the awkward `if case` checks with direct enum
equality assertions: call manager.completionHighlightState(workspaceId:
workspaceId, paneId: currentPaneId) and assert it equals
CompletionPaneHighlightState.current (and similarly assert equals .queued for
queuedPaneId) using XCTest assertion helpers (e.g. XCTAssertEqual); ensure
CompletionPaneHighlightState conforms to Equatable if it does not already so the
direct equality assertion compiles.
In `@Tests/ShellraiserTests/WorkspaceTestSupport.swift`:
- Line 126: Add a brief inline comment on the line that calls
NSApplication.shared (the _ = NSApplication.shared invocation) explaining that
this forces AppKit to initialize (loads the shared NSApplication instance and
sets up AppKit environment for tests that interact with UI components) so future
maintainers understand why the no-op assignment is required.
- Around line 14-34: The test helper makePersistenceContext currently uses
setenv/unsetenv which is not thread-safe; update WorkspacePersistence to accept
an injected appSupportSubdirectory (or a full appSupport URL) via a new
initializer or factory (e.g., add init(appSupportSubdirectory: String) or
init(appSupportURL: URL) to WorkspacePersistence), then change
makePersistenceContext to construct the subdirectory and pass it directly to
WorkspacePersistence instead of calling setenv/unsetenv; remove the environment
manipulation and teardown that unsets those keys and instead keep only the
filesystem cleanup of the created directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d65ca257-aad5-4fe4-987f-b8e44b7623d0
📒 Files selected for processing (18)
.gitignoreMakefilePackage.swiftSources/Shellraiser/Services/Persistence/WorkspacePersistence.swiftTests/ShellraiserTests/AgentCompletionEventMonitorTests.swiftTests/ShellraiserTests/AgentCompletionEventTests.swiftTests/ShellraiserTests/AgentTypeAndWorkspaceModelTests.swiftTests/ShellraiserTests/PaneNodeModelOperationsTests.swiftTests/ShellraiserTests/SurfaceModelTests.swiftTests/ShellraiserTests/WorkspaceCatalogManagerTests.swiftTests/ShellraiserTests/WorkspaceManagerCommandTests.swiftTests/ShellraiserTests/WorkspaceManagerCompletionTests.swiftTests/ShellraiserTests/WorkspaceManagerLifecycleTests.swiftTests/ShellraiserTests/WorkspaceManagerShortcutTests.swiftTests/ShellraiserTests/WorkspacePersistenceTests.swiftTests/ShellraiserTests/WorkspaceSurfaceManagerPaneOperationsTests.swiftTests/ShellraiserTests/WorkspaceSurfaceManagerSurfaceStateTests.swiftTests/ShellraiserTests/WorkspaceTestSupport.swift
| /// Verifies malformed lines are rejected instead of partially parsed. | ||
| func testParseRejectsMalformedCompletionEventLines() { | ||
| XCTAssertNil(AgentCompletionEvent.parse("")) | ||
| XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tcodex")) | ||
| XCTAssertNil(AgentCompletionEvent.parse("invalid-date\tcodex\t00000000-0000-0000-0000-000000001401\t")) | ||
| XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tunknown\t00000000-0000-0000-0000-000000001401\t")) | ||
| XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tcodex\tnot-a-uuid\t")) | ||
| } |
There was a problem hiding this comment.
Add the invalid-base64 case here.
AgentCompletionEvent.parse currently does Data(base64Encoded:) ?? Data() in Sources/Shellraiser/Infrastructure/Agents/CompletionModels.swift:27-43, so a corrupted payload is accepted as an empty string instead of being rejected. This test currently leaves that bug unexercised.
🧪 Tighten the malformed-event coverage
func testParseRejectsMalformedCompletionEventLines() {
XCTAssertNil(AgentCompletionEvent.parse(""))
XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tcodex"))
XCTAssertNil(AgentCompletionEvent.parse("invalid-date\tcodex\t00000000-0000-0000-0000-000000001401\t"))
XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tunknown\t00000000-0000-0000-0000-000000001401\t"))
XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tcodex\tnot-a-uuid\t"))
+ XCTAssertNil(AgentCompletionEvent.parse("2026-03-08T20:00:00Z\tcodex\t00000000-0000-0000-0000-000000001401\tnot-base64"))
}The production parser should also switch to a guarded Data(base64Encoded:) instead of defaulting to Data().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/ShellraiserTests/AgentCompletionEventTests.swift` around lines 20 - 27,
Add a test asserting AgentCompletionEvent.parse rejects a line with an invalid
base64 payload (e.g., a well-formed timestamp/type/uuid followed by a non-base64
string) so the parser fails instead of accepting an empty payload; then update
the parser implementation in AgentCompletionEvent.parse to stop using
Data(base64Encoded:) ?? Data() and instead guard-let the result of
Data(base64Encoded:) (returning nil on failure) so corrupted payloads are
rejected.
| ratio: 0.5, | ||
| first: makeLeaf(paneId: singlePaneId, surfaces: [firstSurface], activeSurfaceId: firstSurface.id), | ||
| second: makeLeaf(paneId: multiPaneId, surfaces: [firstSurface, secondSurface], activeSurfaceId: firstSurface.id) |
There was a problem hiding this comment.
Test data reuses the same surface in two panes.
firstSurface is added to both singlePaneId (line 30) and multiPaneId (line 31). If closeItemTitle or the underlying model uses surface identity for lookups, this could cause subtle test failures or mask bugs. Consider using a distinct surface for the second pane.
🐛 Suggested fix
+ let thirdSurface = makeSurface(
+ id: UUID(uuidString: "00000000-0000-0000-0000-000000001207")!,
+ title: "Third"
+ )
manager.workspaces = [
makeWorkspace(
...
rootPane: .split(
PaneSplitModel(
...
first: makeLeaf(paneId: singlePaneId, surfaces: [firstSurface], activeSurfaceId: firstSurface.id),
- second: makeLeaf(paneId: multiPaneId, surfaces: [firstSurface, secondSurface], activeSurfaceId: firstSurface.id)
+ second: makeLeaf(paneId: multiPaneId, surfaces: [thirdSurface, secondSurface], activeSurfaceId: thirdSurface.id)
)
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/ShellraiserTests/WorkspaceManagerCommandTests.swift` around lines 29 -
31, The test reuses the same Surface instance (firstSurface) in both makeLeaf
calls for singlePaneId and multiPaneId which can hide identity-related bugs;
create a distinct Surface instance (e.g., secondPaneSurface) and use that for
the second makeLeaf (the one building the leaf for multiPaneId) so
closeItemTitle and any model lookups use unique surface identities; update
references in the test setup where makeLeaf(paneId: multiPaneId, surfaces:
[...], activeSurfaceId: ...) is called to pass the new surface and its id
instead of firstSurface.
| try? FileManager.default.removeItem( | ||
| at: FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask)[0] | ||
| .appendingPathComponent(ProcessInfo.processInfo.environment[WorkspacePersistence.appSupportSubdirectoryEnvironmentKey]!, isDirectory: true) | ||
| ) |
There was a problem hiding this comment.
Fragile test setup: force-unwrap on environment variable.
Line 277 force-unwraps the environment key, which will crash if the test runs without the expected environment setup. Consider using a guard or XCTUnwrap for clearer failure diagnostics.
🛡️ Suggested fix
- try? FileManager.default.removeItem(
- at: FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask)[0]
- .appendingPathComponent(ProcessInfo.processInfo.environment[WorkspacePersistence.appSupportSubdirectoryEnvironmentKey]!, isDirectory: true)
- )
+ if let subdirectory = ProcessInfo.processInfo.environment[WorkspacePersistence.appSupportSubdirectoryEnvironmentKey] {
+ try? FileManager.default.removeItem(
+ at: FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask)[0]
+ .appendingPathComponent(subdirectory, isDirectory: true)
+ )
+ }Or use the persistence context's directory directly if available from makePersistenceContext().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/ShellraiserTests/WorkspaceSurfaceManagerSurfaceStateTests.swift` around
lines 275 - 278, The test currently force-unwraps
WorkspacePersistence.appSupportSubdirectoryEnvironmentKey when building the URL
for removal which will crash if the env var is missing; change this to safely
unwrap (using guard let or XCTUnwrap) and fail the test with a clear message if
the environment value is absent, or better yet obtain the directory directly
from makePersistenceContext() (or the persistence context API) and use that
directory for FileManager.default.removeItem to avoid relying on the environment
variable; update any references in this test method to use the safe-unwrapped
value or the persistence context's directory instead of the forced unwrap.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/ShellraiserTests/WorkspacePersistenceTests.swift (1)
5-5: Consider adding edge case coverage.The test suite covers the main scenarios well. For additional robustness, you might consider tests for:
- Empty workspaces array save/load
- Other unsafe override patterns (e.g.,
/absolute,foo/bar,.,.., strings with\)These are optional additions that could be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/WorkspacePersistenceTests.swift` at line 5, Add tests in the WorkspacePersistenceTests class (subclassing WorkspaceTestCase) to cover the edge cases: implement a test like testSaveLoadEmptyWorkspaces that saves an empty workspaces array and then loads it back asserting equality and no errors, and implement a test like testUnsafeOverridePaths that attempts to save/load workspace entries containing unsafe override patterns (e.g., "/absolute", "foo/bar", ".", "..", and strings with backslashes) asserting either proper rejection/validation or expected handling; locate and modify the WorkspacePersistenceTests class and reuse existing helpers from WorkspaceTestCase to create, persist, and load workspaces so the new tests integrate with the current setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Tests/ShellraiserTests/WorkspacePersistenceTests.swift`:
- Line 5: Add tests in the WorkspacePersistenceTests class (subclassing
WorkspaceTestCase) to cover the edge cases: implement a test like
testSaveLoadEmptyWorkspaces that saves an empty workspaces array and then loads
it back asserting equality and no errors, and implement a test like
testUnsafeOverridePaths that attempts to save/load workspace entries containing
unsafe override patterns (e.g., "/absolute", "foo/bar", ".", "..", and strings
with backslashes) asserting either proper rejection/validation or expected
handling; locate and modify the WorkspacePersistenceTests class and reuse
existing helpers from WorkspaceTestCase to create, persist, and load workspaces
so the new tests integrate with the current setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2adaa1b1-8db8-4bd2-ac7d-af32ebf45e19
📒 Files selected for processing (2)
Sources/Shellraiser/Services/Persistence/WorkspacePersistence.swiftTests/ShellraiserTests/WorkspacePersistenceTests.swift
Summary by CodeRabbit
New Features
Tests
Chores