feat(macapp): GUI correctness, safety, and accessibility hardening (epic #991) - #1021
Open
dennisonbertram wants to merge 41 commits into
Open
feat(macapp): GUI correctness, safety, and accessibility hardening (epic #991)#1021dennisonbertram wants to merge 41 commits into
dennisonbertram wants to merge 41 commits into
Conversation
Behavioral tests added: R1 (auto-scroll only near bottom), reachability wiring check. Test runner output (expected: all failing/compile error - TranscriptScrollPin and Layout.autoscrollPinThreshold do not exist yet): error: cannot find 'TranscriptScrollPin' in scope (x6, one per test) error: type 'Layout' has no member 'autoscrollPinThreshold' (x3) error: fatalError (build failed, tests could not run) These tests will pass after the implementation in the next commit.
Implementation for tests added in 808276a. TranscriptScrollPin is a pure value type (starts pinned; update(distanceFromBottom:) unpins past Layout.autoscrollPinThreshold, inclusive at the boundary; re-pins on return to 0; stays pinned on negative/overscroll distance). TranscriptView wires it via a PreferenceKey carrying the bottom anchor's frame in a named coordinate space attached to the ScrollView, feeding (anchorMinY - viewport height) into pin.update(distanceFromBottom:); scrollIfPinned now guards on pin.isPinned instead of the previously never-mutated pinnedToBottom constant. Test runner output (expected: all passing): ✔ Test "a fresh pin starts pinned to the bottom" passed ✔ Test "staying at distance 0 keeps the pin pinned" passed ✔ Test "scrolling past the threshold unpins autoscroll" passed ✔ Test "distance exactly at the threshold is still pinned" passed ✔ Test "scrolling back to the bottom re-pins autoscroll" passed ✔ Test "overscroll bounce reports a negative distance and stays pinned" passed ✔ Test "transcript autoscroll is actually wired to a live scroll pin" passed Test run with 181 tests in 41 suites passed (up from 174/40 baseline) Behavioral tests covered: R1 (happy/edge/boundary/overscroll cases), reachability wiring check. Files changed: macapp/Sources/GoCodeUI/TranscriptScrollPin.swift (new), macapp/Sources/GoCodeUI/ChatView.swift, macapp/Sources/GoCodeUI/DesignSystem/Layout.swift
Regression test added that would fail if the geometry plumbing behind the scroll pin in f90417b is stripped out while leaving the pin.update(...) call site textually intact (e.g. replaced with a hardcoded distance) - a gap the red-commit wiring test alone does not close, since it only checks the consumer side. Full test suite output: Test run with 182 tests in 41 suites passed after ~5.1 seconds (0 failures; up from 174 tests / 40 suites baseline) Regression scenarios covered: - ChatView.swift declares .coordinateSpace(name: scrollSpace) on the ScrollView - The bottom anchor's frame is carried by TranscriptBottomAnchorKey - pin.update(distanceFromBottom:) is fed via .onPreferenceChange(TranscriptBottomAnchorKey.self), not a placeholder constant
…y (macapp) Behavioral tests added: CollectionLoadState state-table extension (failed carries a message, showsError, showsPlaceholder excludes failed) and ProjectSession-level stub-driven tests (per-collection error message, retry recovery, previously-loaded rows survive a failure, per-collection isolation), plus a CollectionErrorState reachability check. Test runner output (expected: all failing — compile-time red, since CollectionLoadState.failed has no associated value yet): error: enum case 'failed' has no associated values error: value of type 'CollectionLoadState' has no member 'showsPlaceholder' error: value of type 'CollectionLoadState' has no member 'errorMessage' error: value of type 'CollectionLoadState' has no member 'showsError' error: fatalError These tests will pass after the implementation in the next commit.
…retry Implementation for tests added in 728107a. CollectionLoadState.failed now carries the server's own message, plus showsError and showsPlaceholder(itemCount:) helpers so views stop hand-rolling `state != .loaded` skeleton checks. A failed load is deliberately excluded from showsPlaceholder — the bug was a failure rendering identically to an endless loading skeleton. New CollectionErrorState view (icon + verbatim server message + Retry), mirroring StartupFailureView's existing "this failed, here is why, retry" shape at inline/per-collection scale. Wired into every consumer: task/run sections in ActivityView, the conversations list and checkpoints in SessionsView, providers/models tabs in SettingsView, and the provider and model lists in ModelSettingsView (which also gained a per-collection failed(message) instead of a bare failure marker). ProjectSession's eight `= .failed` assignments now carry error.localizedDescription; the existing statusMessage toast write is unchanged, so ProjectSessionActivityTests still pass unmodified in intent. Test runner output (expected: all passing): swift build: Build complete! swift test: Test run with 192 tests in 43 suites passed after 4.935 seconds. swift format lint --strict --recursive Sources Tests: clean (no output) Behavioral tests covered: R2 (CollectionLoadStateTests state-table extension; ProjectSessionLoadStateTests per-collection message, retry recovery, per-collection isolation, previously-loaded rows survive a failure; CollectionErrorState reachability). Files changed: - macapp/Sources/GoCodeUI/DesignSystem/CollectionLoadState.swift - macapp/Sources/GoCodeUI/ProjectSession.swift - macapp/Sources/GoCodeUI/ActivityView.swift - macapp/Sources/GoCodeUI/SessionsView.swift - macapp/Sources/GoCodeUI/SettingsView.swift - macapp/Sources/GoCodeUI/ModelSettingsView.swift - macapp/Tests/GoCodeUITests/ProjectSessionLoadStateTests.swift (format-only)
…wiring Regression tests added that would fail if the change in b3bb036 is reverted. Two angles distinct from the behavioral tests already covering CollectionLoadState's own logic and ProjectSession's per-collection messages: - Per-file wiring: the existing module-wide reachability check is satisfied the moment any one view wires CollectionErrorState in, so a revert that drops the wiring from five of the six U2 consumers while leaving it in the sixth would pass silently. A new test pins ActivityView, SessionsView, SettingsView, and ModelSettingsView individually, each for both `CollectionErrorState(` and `.showsError`. - A different integration point: ModelSettingsModel holds its own CollectionLoadState via client.modelSettings(), never exercised by the ProjectSession-driven tests (which cover client.models()/providers()/ conversations()). Catches a revert of ModelSettingsView.swift's `.failed(error.localizedDescription)` back to a bare `.failed`. Kept the new ModelSettingsModel test inside the existing `.serialized` ProjectSessionLoadStateTests suite rather than a suite of its own: both share LoadStateStubProtocol's single global handler, and a separate suite running concurrently with this one raced it (observed one flaky failure before the merge; stable across 3 repeated full-suite runs after). Full test suite output: swift build: Build complete! swift test (x3 consecutive runs): Test run with 194 tests in 43 suites passed after ~4.6-4.9 seconds each run, 0 failures. swift format lint --strict --recursive Sources Tests: clean (no output) Regression scenarios covered: - Partial revert of the per-view CollectionErrorState wiring (any one of the four remaining consumer files). - Reversion of ModelSettingsModel.load()'s failure branch to a bare .failed with no message.
… (macapp) Behavioral tests added: R3 (cancel/approve/deny/answerInput surface their server acknowledgement, pending questions clear only after the server accepts) and R4 (an answer set is submittable only when every question has a non-blank answer). RunControlAckTests.swift drives RunSession against a method+path-keyed HTTP/SSE stub (approve/deny/cancel failures surface via connectionError; a 409 on answerInput leaves pendingQuestions non-nil; a failed cancel does not let a second press force-abandon the stream). AskUserAnswersTests.swift covers the new AskUserAnswers.isComplete predicate directly. Test runner output (expected: all failing -- compile-time red, since AskUserAnswers does not exist yet): error: cannot find 'AskUserAnswers' in scope (x5, one per test) error: fatalError (build failed, tests could not run) These tests will pass after the implementation in the next commit.
Implementation for tests added in 784bb19. RunSession.cancel/approve/deny/answer now await the server call inside a do/catch, matching the pattern steer() already used correctly: a HarnessError sets connectionError to its message, a transport error sets it to localizedDescription. A failed cancel resets cancelRequested to false so the operator's next press retries the cooperative request rather than escalating to a local force-kill. answer(_:) validates completeness against pendingQuestions via the new AskUserAnswers.isComplete and clears pendingQuestions only after answerInput returns successfully -- a rejected answer leaves the prompt on screen. New AskUserAnswers.isComplete(prompt:answers:) replaces `answers.count < prompt.questions.count`, which counted a field cleared back to "" (or left whitespace-only) as answered. Shared by RunSession's guard and AskUserView's Send-button `disabled` predicate in ChatView.swift so both read the same rule. Only whitespace reformatting (swift-format) applied to the red-committed RunControlAckTests.swift; no assertions changed. Test runner output (expected: all passing): swift build: Build complete! swift test: Test run with 205 tests in 45 suites passed after 4.698 seconds. swift format lint --strict --recursive Sources Tests: clean (no output) Behavioral tests covered: R3 (RunControlAckTests -- approve/deny/cancel failure surfacing, answerInput 409 keeps pendingQuestions, cancel-retry stays cooperative), R4 (AskUserAnswersTests -- completeness predicate). Files changed: Sources/GoCodeUI/AskUserAnswers.swift (new), Sources/GoCodeUI/RunSession.swift, Sources/GoCodeUI/ChatView.swift
…ent wiring Regression tests added that would fail if the change in ef88bef is reverted. Confirmed against the pre-fix source at 8f2e412: RunSession.swift there still contains `try? await client.cancel/approve/deny/answerInput`, and ChatView.swift there still contains `answers.count < prompt.questions.count`. Two angles distinct from the behavioral tests already covering the runtime behavior: - RunControlAckTests exercises each of cancel/approve/deny/answer through a live stub, proving the *behavior* is fixed, but a partial revert of one call site back to `try?` would only be caught if that specific method's test happened to still be run against the reverted code -- this pins the absence of the bug shape across the whole file in one assertion. - AskUserAnswersTests pins the predicate's own logic in isolation but does not prove AskUserView's Send button still calls it; a revert that reintroduces `answers.count < prompt.questions.count` while leaving AskUserAnswers.swift untouched (and its own tests passing) would slip through that suite alone. Full test suite output: swift build: Build complete! swift test: Test run with 207 tests in 45 suites passed after 5.744 seconds (0 failures; up from 194 baseline / 205 after the green commit). swift format lint --strict --recursive Sources Tests: clean (no output) Regression scenarios covered: - Reversion of any of RunSession's four fixed call sites back to `try?` - Reversion of AskUserView's Send-button predicate to the pre-fix count comparison
Behavioral tests added: newConversation/fork/undo refuse while a run is active (R5); idle path unaffected; guard clears once the run completes; deleteConversation's internal newConversation() call inherits the guard. Test runner output (expected: 5 of 6 failing, right reason -- server calls went through / state mutated / no refusal message): Test Suite 'ProjectSession lifecycle guard' failed after 0.138 seconds with 10 issues. ✘ newConversation refuses while a run is active -- core regression: conversationID nil != "conv_1", isBusy false != true, statusMessage nil != contains "running" ✘ fork refuses while a run is active and never reaches the server: fork request WAS recorded, statusMessage missing "running" ✘ undo refuses while a run is active and never reaches the server: undo request WAS recorded, statusMessage missing "running" ✔ with no active run, fork/undo/newConversation behave exactly as before (idle path unaffected -- passes today, pinned) ✘ a previously refused fork succeeds once the run completes: fork request WAS recorded while still busy ✘ deleteConversation's internal reset inherits the guard for its own busy run: conversationID reset to nil instead of refused These tests will pass after the implementation in the next commit.
…e run Implementation for tests added in a19fd39. newConversation/fork/undo now refuse (with a statusMessage naming the action) while run?.isBusy is true, at the shared ProjectSession boundary (KTD-9) so every one of the six call sites is covered by a single guard, including deleteConversation's own internal newConversation() call. Test runner output (expected: all passing): ✔ newConversation refuses while a run is active -- core regression ✔ fork refuses while a run is active and never reaches the server ✔ undo refuses while a run is active and never reaches the server ✔ with no active run, fork, undo, and newConversation behave exactly as before ✔ a previously refused fork succeeds once the run completes -- guard is state-based, not sticky ✔ deleteConversation's internal reset inherits the guard for its own busy run Test run with 6 tests in 1 suite passed after 0.070 seconds. Full suite: 213 tests in 46 suites passed (up from 207 baseline). Behavioral tests covered: R5 (newConversation/fork/undo refuse during an active run, at the shared ProjectSession boundary). Files changed: macapp/Sources/GoCodeUI/ProjectSession.swift
Regression tests added that would fail if the guard in 540b392 is reverted or narrowed to a subtly wrong predicate. Full test suite output: Test run with 215 tests in 46 suites passed after 5.450 seconds. Regression scenarios covered: - The guard reads run?.isBusy == true, not run?.isBusy != false: before a project ever connects, run is nil, and nil must never be treated as busy -- newConversation/fork/undo all proceed as no-ops with no false-positive statusMessage. - Each of the three refusal messages names its own action and is distinct from the other two, so a revert to one shared generic string would be caught even though it still mentions "a run is active".
…iews Behavioral tests added: DeletePreview names the title and a known message count and never fabricates one (R6); a very long title is bounded; UndoPreview quotes the last user prompt or reads neutrally with none, and flattens a multi-line prompt; lastUserPrompt finds the most recent .userPrompt item in a transcript; reachability check that delete/undo route through the shared destructiveConfirmation( presentation instead of firing immediately. Test runner output (expected: compile-time red -- DeletePreview/UndoPreview do not exist yet, matching the U1/U3 precedent for brand-new value types): error: cannot find 'DeletePreview' in scope (x3) error: cannot find 'UndoPreview' in scope (x6) error: fatalError (build failed, tests could not run) These tests will pass after the implementation in the next commit.
Implementation for tests added in 9fa78d3. Builds the shared DestructiveConfirmation presentation (KTD-4) plus DeletePreview/UndoPreview, pure client-derived preview builders (KTD-5 -- no server dry-run exists, so text is composed from ConversationInfo.messageCount and the transcript's last .userPrompt, never a fabricated count). Wires all four undo entry points (SettingsView ProjectTab, ChatView MessageActions, ConversationChrome menu, and SessionsView's own delete flow) plus the conversation delete menu item through the shared confirmation instead of firing immediately. Cancel performs no mutation and no server call. Test runner output (all passing): ✔ Test run with 9 tests in 1 suite passed after 0.008 seconds. (Suite "Destructive confirmation previews") Full-suite check: 224 tests in 47 suites passed (0 failures), up from 215 at the U4 baseline. swift build and swift format lint --strict --recursive Sources Tests both clean. Behavioral tests covered: R6 (delete/undo confirmations name what is lost). Files changed: - macapp/Sources/GoCodeUI/DesignSystem/DestructiveConfirmation.swift (new) - macapp/Sources/GoCodeUI/SessionsView.swift - macapp/Sources/GoCodeUI/SettingsView.swift - macapp/Sources/GoCodeUI/ChatView.swift - macapp/Sources/GoCodeUI/ConversationChrome.swift - macapp/Tests/GoCodeUITests/DestructiveConfirmationTests.swift
…irmation wiring Regression tests added that would fail if the wiring in 7674eaf is reverted. The green commit's module-wide reachability test scans for "destructiveConfirmation(" anywhere in Sources/GoCodeUI, so it would still pass if three of the four undo entry points (ChatView, ConversationChrome, SettingsView) reverted to calling project.undo() directly while only one site kept the fix. These tests pin each site by name (occurrence-counting confirmUndo()/confirmDelete() rather than mere containment, since a bare containment check would still pass against a dangling now-unused declaration -- verified by manually reverting ChatView's undo button back to `Task { await project.undo() }`: the new test failed with "occurrences(of: "confirmUndo()", ...) -> 1 >= 2", then passed again once restored). Full test suite output: ✔ Test run with 226 tests in 47 suites passed after 4.849 seconds. (224 after the green commit; +2 regression tests here. 215 at the U4 baseline.) swift build and swift format lint --strict --recursive Sources Tests both clean. Regression scenarios covered: - Each of the three MessageActions/ConversationHeader/ProjectTab undo call sites invokes its own confirmUndo() and builds its message from UndoPreview, not a bare project.undo() call. - SessionsView's delete menu item invokes confirmDelete(conversation) and DeletePreview.message(for:), not deleteConversation() directly.
Behavioral tests added: a rewind_refused refusal is captured structurally via ProjectSession.rewindRefusal, matched on HarnessError.code (KTD-6), not HTTP status (R7); a generic failure sets statusMessage and offers no force path; confirming sends force:true on the second request and clears the refusal on success; a second refusal on the forced call sets rewindRefusal again rather than looping or clearing silently; a successful first attempt reports counts with no refusal (existing behaviour, pinned); reachability check that SessionsView wires force: true only inside the refusal- confirmation branch (2 occurrences of rewind(to:, 1 of force: true) and the stale #951 finding-9 NOTE is gone. Test runner output (expected: compile-time red -- ProjectSession.rewindRefusal and RewindRefusal do not exist yet, matching the U1/U3/U5 precedent for brand-new observable state): error: value of type 'ProjectSession' has no member 'rewindRefusal' (x9) error: fatalError (build failed, tests could not run) These tests will pass after the implementation in the next commit.
…force confirmation Implementation for tests added in 91d861a. ProjectSession.swift: adds `RewindRefusal` (point + server message) and `rewindRefusal` observable state, cleared at the start of every `rewind` call (including its own retry) so a stale refusal from a prior point can never bleed into a new one. `rewind`'s catch now branches on `HarnessError.code == "rewind_refused"` (KTD-6 -- the code is the stable contract, not the HTTP status it happens to arrive with) and records the refusal instead of collapsing it into `statusMessage` prose; every other `HarnessError` still lands in `statusMessage` as before. `dismissRewindRefusal()` clears a refusal with no server call, for the confirmation's Cancel path. SessionsView.swift: `CheckpointsView` retires the stale #951 finding-9 NOTE and presents a second `DestructiveConfirmation` (U5's shared component) keyed off `project.rewindRefusal` -- worded distinctly from the ordinary restore alert (names the file-changed-outside-the-harness fact, quotes the server's message, confirm label "Restore Anyway") and calls `rewind(to:refusal.point, force: true)` only from that branch. Declining calls `dismissRewindRefusal()`; nothing is ever auto-retried with force. Also fixes a reentrant-lock deadlock in the new test file's stub: a handler that calls `RewindStub.bodies(matching:)` to count prior attempts was being invoked while `startLoading()` still held the same non-reentrant `NSLock`. Test runner output (all passing): ✔ Test "a rewind_refused refusal is captured structurally, matched on HarnessError.code -- core regression" passed ✔ Test "a generic failure sets statusMessage and offers no force path" passed ✔ Test "confirming the refusal sends force:true, and the refusal clears once the forced call succeeds" passed ✔ Test "a second refusal on the forced call sets rewindRefusal again rather than looping or clearing silently" passed ✔ Test "a successful rewind reports the restore counts with no refusal -- existing behaviour, pinned" passed ✔ Test "SessionsView wires force: true only inside the refusal-confirmation branch, and the stale NOTE is gone" passed ✔ Test run with 232 tests in 48 suites passed after 24.561 seconds. (226 at the U5 baseline; +6 here.) swift build and swift format lint --strict --recursive Sources Tests both clean. Behavioral tests covered: R7 (rewind_refused surfaced structurally, distinct force confirmation, never auto-retried). Files changed: macapp/Sources/GoCodeUI/ProjectSession.swift, macapp/Sources/GoCodeUI/SessionsView.swift, macapp/Tests/GoCodeUITests/ProjectSessionRewindTests.swift
…iring Regression tests added that would fail if the wiring in 0c15cf3 is reverted -- each was manually verified red against a targeted revert, then restored and re-verified green: - "dismissing a refusal clears it without contacting the server": reverted dismissRewindRefusal() to clear rewindRefusal without also asserting no new request fired would not have caught a Cancel path that silently re-issued rewind(force: false); this pins the request count too. - "attempting a rewind on a different checkpoint clears a stale refusal from a prior one": reverting the `rewindRefusal = nil` at the top of `rewind` (leaving it cleared only via the success/failure branches) failed this test with "RewindRefusal(... point_a ...) == nil" while the green commit's own tests still passed -- they only ever act on a single point, so none of them exercised the start-of-call clear. - "the force-rewind confirmation's decline path calls dismissRewindRefusal": blanking SessionsView's Cancel-path call to `project.dismissRewindRefusal()` failed this test while the green commit's module-wide "force: true"/NOTE- removal reachability check still passed unaffected, since that check never looks at the decline path at all. Full test suite output: ✔ Test run with 235 tests in 48 suites passed after 5.361 seconds. (232 after the green commit; +3 regression tests here. 226 at the U5 baseline / start of this task.) swift build and swift format lint --strict --recursive Sources Tests both clean. Regression scenarios covered: - Cancelling the force-rewind confirmation never contacts the server. - A stale refusal from a previously-refused checkpoint cannot leak into a fresh rewind attempt on a different checkpoint. - The confirmation's decline path is wired to a real dismissal, not a dangling no-op.
Behavioral tests added: Up walks backwards through recorded prompts, newest first; Up at the oldest entry stays put (no wraparound, no nil-after-start); Down walks forward and restores the exact stashed draft, including an empty one, then declines further; a half-typed draft declines a recall and leaves the cursor unmoved (core regression, R8); recording mid-navigation resets the cursor so the next Up starts from the newest; empty history declines without crashing; duplicate consecutive prompts are both recorded, not deduped; reset clears navigation without touching recorded entries; reachability check that ChatView wires .onKeyPress(.upArrow / .downArrow to production recall (closes the #927/#998 gap where recallPreviousPrompt had no call site). Test runner output (expected: compile-time red -- PromptHistory does not exist yet, matching the U1/U3/U5/U6 precedent for brand-new value/observable state): error: cannot find 'PromptHistory' in scope (x9) error: fatalError (build failed, tests could not run) These tests will pass after the implementation in the next commit.
Implementation for tests added in 9a495ce. PromptHistory (new, Sources/GoCodeUI/PromptHistory.swift) is a pure value type: entries + an optional cursor + a stashed pre-recall draft. recallPrevious(currentDraft:) starts navigation only from an empty draft (stashing it), walks backwards with no wraparound past the oldest entry, and -- once already navigating -- declines instead of overwriting an edit that has diverged from the entry last recalled (KTD-7's caret-awareness approximation, since TextSelection/caret binding is a macOS 15 API and this package floors at .macOS(.v14)). recallNext() walks forward and restores the exact stashed draft past the newest entry, then declines. record(_:) always resets navigation. RunSession replaces `promptHistory: [String]` with a `PromptHistory`, records on submit(), and exposes recallPreviousPrompt() / recallNextPrompt() (Bool return: whether something was recalled) plus noteManualDraftEdit() so a manual edit after a recall clears navigation rather than letting the next arrow press silently replace it. ChatView's Composer wires .onKeyPress(.upArrow/.downArrow) on the draft field, returning .handled only when the session actually recalled something so the field's own multi-line navigation still works otherwise, and calls noteManualDraftEdit() from the existing onChange(of: run.draft) for any change the recall handlers did not themselves cause -- closing the #927/#998 gap where recallPreviousPrompt had no production call site. Test runner output (all passing): Test run with 10 tests in 1 suite passed after 0.002 seconds. (PromptHistory cursor navigation) Full suite: 245 tests in 49 suites passed (up from the 235-test baseline). swift build clean. swift format lint --strict --recursive Sources Tests clean. Behavioral tests covered: all 9 PromptHistoryTests scenarios from the plan's U7 section plus the mandated reachability check. Files changed: macapp/Sources/GoCodeUI/PromptHistory.swift, macapp/Sources/GoCodeUI/RunSession.swift, macapp/Sources/GoCodeUI/ChatView.swift
…iring Regression tests added that would fail if the change in c179c2c is reverted: - PromptHistory: Up declines without clobbering once the recalled entry has been edited (KTD-7's already-navigating check) -- verified this fails when that check is removed. - RunSession: noteManualDraftEdit() resets navigation so Down cannot silently overwrite an edit made after a recall. recallNext() takes no draft parameter by the plan's own PromptHistory signature, so without this reset it would restore the pre-recall stash over the edit instead of declining -- verified by temporarily no-opping noteManualDraftEdit() and confirming the test fails ("Expectation failed: (session.draft -> "") == "first prompt, but edited""), then restoring the fix and confirming it passes again. - RunSession wiring: submit() records so a later Up recalls it, and recallPreviousPrompt() declines on empty history -- exercises the RunSession-level integration the pure PromptHistory tests can't reach (a single submit() only, since a second synchronous submit() in the same test is blocked by the isBusy guard Transcript sets optimistically on appendUserPrompt). Full test suite output: Test run with 249 tests in 50 suites passed after 5.160 seconds. (Baseline before U7: 235 passing. After green: 245. After this commit: 249 -- strictly increasing per the plan's Verification Contract.) swift build clean. swift format lint --strict --recursive Sources Tests clean. Regression scenarios covered: - Up never replaces an in-navigation edit that no longer matches the recalled entry. - Down never replaces an edit made after a recall, once the composer has reported the manual edit via noteManualDraftEdit().
…edback
Behavioral tests added: rows/toggles as real accessible controls (R9),
model-settings status feedback surviving its own reload (R10).
Test runner output (expected: failing before implementation):
Building for debugging...
[0/8] Write sources
[1/8] Write swift-version--58304C5D6DBC2206.txt
[3/7] Emitting module GoCodeUITests
[4/7] Compiling GoCodeUITests AccessibilityReachabilityTests.swift
[5/7] Compiling GoCodeUITests ModelSettingsFeedbackTests.swift
/Users/dennison/develop/go-code/macapp/Tests/GoCodeUITests/ModelSettingsFeedbackTests.swift:153:42: error: argument passed to call that takes no arguments
151 | model.status = "some stale message"
152 |
153 | await model.load(clearingStatus: true)
| `- error: argument passed to call that takes no arguments
154 |
155 | #expect(model.status == nil)
error: fatalError
This is a genuine "feature does not exist yet" compile failure, not an
import/typo error: the test calls ModelSettingsModel.load(clearingStatus:),
a signature this unit adds. The whole target fails to build until the
signature lands, exactly as the plan's own K1 risk note documents for the
analogous CollectionLoadState.failed(String) change in U2 ("the enum
change... breaks compilation across the six views — expected and
intended"). The other 9 tests in these two files are plain string/behavior
assertions against current (pre-fix) source and fail meaningfully on their
own once compilation succeeds (verified by reading current SessionsView.swift
line 53 .onTapGesture, SettingsView.swift line 166 .onTapGesture, and
ModelSettingsView.swift's unconditional `status = nil` on load success and
missing .accessibilityLabel on the exposure Toggle and confirmRemove helper).
These tests will pass after the implementation in the next commit.
… feedback Implementation for tests added in 93867ce. - SessionsView.swift: conversation rows are now a Button wrapping ConversationRow (.buttonStyle(.plain) keeps the look), replacing .onTapGesture, with an .accessibilityLabel naming title, message count, and pinned state. The context menu is unchanged. - SettingsView.swift: ModelsTab rows are likewise a Button with an .accessibilityLabel naming the model id, provider, and selection state, replacing .onTapGesture { project.selectedModel = model.id }. - ModelSettingsView.swift: - ModelSettingsModel.load(clearingStatus: Bool = true) replaces load(). The .task initial load keeps the default (clears any stale status); fetch/setExposed/setAllVisible/saveProvider/delete now call load(clearingStatus: false) on both success and failure so the message they just set survives the reload they trigger instead of being erased by it. - The exposure Toggle gains .accessibilityLabel("Show \(modelID) in the picker") alongside its existing .labelsHidden() + .help(...). - Provider "Remove" now calls a new confirmRemove(_:) that presents a DestructiveConfirmation (from U5) stating that removal drops the provider's exposed models, instead of calling model.delete immediately from the button action. Test runner output (full suite, expected: all passing): Test run with 259 tests in 52 suites passed after 5.880 seconds. (249 baseline + 10 new: 5 in "ModelSettingsModel status feedback", 5 in "Accessibility and settings-feedback reachability") Behavioral tests covered: all 10 tests in Tests/GoCodeUITests/ModelSettingsFeedbackTests.swift and Tests/GoCodeUITests/AccessibilityReachabilityTests.swift. Files changed: - macapp/Sources/GoCodeUI/SessionsView.swift - macapp/Sources/GoCodeUI/SettingsView.swift - macapp/Sources/GoCodeUI/ModelSettingsView.swift
… status feedback Regression tests added that would fail if the change in bd6d7dd is reverted, covering angles distinct from the red-commit behavioral tests: - ModelSettingsFeedbackTests: a failed delete()'s status message survives the load(clearingStatus: false) reload its own catch branch triggers -- a different mutating method than the fetch/setExposed tests already covered, so a fix that special-cased only those two would still be caught. - AccessibilityReachabilityTests: the conversation row's and model row's accessibility labels are attached to an actual Button wrapping the row content (verified via source-scan regex spanning the Button/label closure), not merely present somewhere else in the file as a bystander. The existing "no .onTapGesture" checks alone would not catch a row left inert with an unattached label. Full test suite output: Test run with 262 tests in 52 suites passed after 5.180 seconds. Regression scenarios covered: - delete() failure message surviving its own reload (distinct from fetch/setExposed) - conversation row accessibility label bound to a real Button, not a bystander - model row accessibility label bound to a real Button, not a bystander
…ility test scaffolding RunSession: approve/deny/steer now share one runControlTask helper for the error-surfacing catch shape; cancel/answer keep their own do/catch since they touch other state on failure. promptHistory drops its public accessor since nothing outside RunSession reads it. Tests: AccessibilityReachabilityTests and TranscriptFeatureReachabilityTests had four near-identical copies of "read a source file / read the whole module" helpers. Extracted to Tests/GoCodeUITests/ReachabilitySource.swift. Baseline: 262 tests passing before and after (no behavior change).
Behavioral tests added for eight verified review findings:
F1a: answer() must guard against a second call while the first is
still in flight (only one POST reaches the server).
F1b: a success answer must only clear pendingQuestions when it is
still the very prompt that was answered (a newer prompt assigned
mid-flight must survive).
F2: a second cancel() press while the first cooperative cancel is
still in flight must not escalate to a local force-stop.
F3: a cancel Task that resolves after reset() must not write a stale
connectionError for the abandoned run.
F4: deleteConversation must refuse up front for its own busy
conversation -- the DELETE must never reach the server.
F5: fork/undo must re-check busyness after their server call, before
applying rebind/reload, so a run started mid-flight is not
clobbered.
F6: openConversation must refuse while a run is active.
F7: PromptHistory.recallPrevious must decline (nil) rather than
repeat a same-value no-op at the oldest entry -- a same-value
draft reassignment never fires SwiftUI's onChange, which left the
composer's isRecallingHistory flag stuck and misattributed the
next real edit.
F8: the transcript's scroll pin must ignore geometry updates while
its own scrollTo animation is in flight (and before a real
viewport height is known), or it self-unpins from the very scroll
it triggered.
Test runner output (expected: all new tests failing meaningfully,
zero regressions in the 262-test baseline):
✘ Suite "PromptHistory cursor navigation" failed with 1 issue.
✘ Suite "Transcript feature reachability" failed with 5 issues.
✘ Suite "ProjectSession lifecycle guard" failed with 6 issues.
✘ Suite "RunSession control acknowledgements" failed with 4 issues.
Test run with 273 tests in 52 suites failed after 4.784 seconds
with 16 issues.
(16 issues == 4 RunControlAckTests + 6 ProjectSessionLifecycleGuardTests
+ 1 PromptHistoryTests + 5 TranscriptFeatureReachabilityTests --
exactly and only the 11 new tests; every one of the 262 baseline
tests still passes.)
Sample failures, each meaningful (not an import/compile error):
- PromptHistoryTests.swift:134: recallPrevious(currentDraft: "a")
returned "a" instead of declining with nil.
- RunControlAckTests.swift:351: session.transcript.runState became
.cancelled during an in-flight cooperative cancel.
- RunControlAckTests.swift:407: a stale connectionError
("cancel rejected") was written after reset().
- RunControlAckTests.swift:489/576: a second answer() sent a
duplicate POST, and pendingQuestions was cleared to nil instead
of surviving as call_2.
- ProjectSessionLifecycleGuardTests.swift:268/296-298/380/426: the
DELETE/messages/undo-reload calls reached the server, and
fork/openConversation applied their result, while a run was busy.
- TranscriptFeatureReachabilityTests.swift:104-105/143-150: ChatView.swift
does not yet contain the answerInFlight-gated Send button or the
isAutoScrolling guard (these two are source-scan tests, matching
this file's existing pattern for view-layer state not otherwise
assertable through a headless test on this stack).
These tests will pass after the implementation in the next commit.
Implementation for tests added in dd0ae51. RunSession.swift: - answer() gains an answerInFlight guard (cleared via defer on every exit path) so a second call while the first is in flight is a no-op; a success only clears pendingQuestions when it is still the prompt this call answered (compared by callID), so a newer question assigned mid-flight survives. - cancel() replaces the cancelRequested bool (set true synchronously, before the request resolved) with an explicit CancelState (.idle/ .requesting/.requested) state machine, so a second press only escalates once the server has actually acknowledged the first cooperative cancel. - cancel()'s and answer()'s Tasks capture their runID and guard every connectionError/pendingQuestions/cancelState write on currentRunID still matching it, so a Task that resolves after reset() cannot write stale state into a new run/conversation. reset() now also resets cancelState and answerInFlight directly, since the guard above only stops a stale Task from overwriting NEW state -- it does not clear old state itself. ProjectSession.swift: - deleteConversation refuses up front (distinct message) when the target conversation is the current, busy one -- the DELETE no longer reaches the server before the local refusal, unlike before when the delete happened and only the internal newConversation() call refused. - fork/undo re-check busyness after their server call, before applying rebind/reload, so a run started on the same conversation while the request was in flight is not clobbered. - openConversation gains the same refuseIfBusy guard as newConversation/ fork/undo. PromptHistory.swift: - recallPrevious declines (nil) instead of repeating a same-value no-op when already at the oldest entry and the draft already shows it. A same-value reassignment never fires SwiftUI's onChange, which left the composer's isRecallingHistory flag stuck and misattributed the next real edit. ChatView.swift / DesignSystem/Motion.swift: - TranscriptView gains an isAutoScrolling flag, set for the duration of scrollIfPinned's scrollTo animation (Motion.autoscrollDuration, replacing the prior 0.12 literal). pin.update is skipped while that flag is set or before scrollViewportHeight reports a real value, so the pin can no longer self-unpin from its own programmatic scroll. TranscriptScrollPin itself is untouched -- it stays a pure decision. - AskUserView takes answerInFlight and disables Send while it is true. Test runner output (expected: all passing): ✔ Suite "PromptHistory cursor navigation" passed ✔ Suite "Transcript feature reachability" passed ✔ Suite "ProjectSession lifecycle guard" passed ✔ Suite "RunSession control acknowledgements" passed Test run with 273 tests in 52 suites passed after 11.334 seconds. swift format lint --strict --recursive Sources Tests: clean. Behavioral tests covered: F1a, F1b, F2, F3, F4, F5, F6, F7, F8. Files changed: Sources/GoCodeUI/RunSession.swift, Sources/GoCodeUI/ProjectSession.swift, Sources/GoCodeUI/PromptHistory.swift, Sources/GoCodeUI/ChatView.swift, Sources/GoCodeUI/DesignSystem/Motion.swift, Tests/GoCodeUITests/RunControlAckTests.swift (swift-format only), Tests/GoCodeUITests/TranscriptFeatureReachabilityTests.swift (reformat one assertion to two format-resilient substring checks after swift-format re-wrapped the .disabled(...) call it is scanning for).
…0d9f Regression tests added that would fail if the changes in 4250d9f are reverted, each covering an angle distinct from the F1-F8 behavioral tests: - RunSession.resetClearsAnswerInFlightForLaterConversations: proves reset() itself clears answerInFlight, not just that answer()'s guard works while in flight (the existing F1a/F1b tests). A revert that keeps the answer() guard but drops `answerInFlight = false` from reset() would pass every existing test (none of them call answer() again after a reset()) while leaving Send permanently disabled for every conversation opened after one whose answer never came back. Verified this fails against the pre-fix reset() (manually reverted the one line, ran the test, restored it) -- confirms it actually exercises the fix, not a tautology. - ProjectSession.deleteConversationSucceedsForUnrelatedConversationWhileAnotherIsBusy: proves the F4 guard is conjunctive (conversation.id == run?.conversationID AND busy), not a simplified run?.isBusy == true that would still pass the existing "refuses its own busy conversation" test while wrongly blocking every other conversation's delete for as long as anything is running. Verified this fails against that exact simplification (manually applied it, ran the test, restored the conjunctive guard). Full test suite output: Test run with 275 tests in 52 suites passed after 5.099 seconds. swift build: clean. swift format lint --strict --recursive Sources Tests: clean. Regression scenarios covered: - answerInFlight left stuck true across conversations after an abandoned in-flight answer. - deleteConversation's busy guard over-blocking unrelated conversations.
RunSession.runControlTask (shared by approve/deny/steer) writes connectionError on failure with no guard against a Task that resolves after reset() has already moved the session onto a different (or no) run -- the same stale-write race already fixed for cancel/answer. Test runner output (expected: failing): Test "an approve Task that resolves after reset() must not surface a stale connectionError -- core regression" failed after 0.138 seconds with 1 issue. Expectation failed: (session.connectionError -> "approve rejected") == nil an approve response arriving after reset() must not resurrect state for an abandoned run This test will pass after the guard is added to runControlTask in the next commit.
Implementation for the test added in e8e0651. runControlTask (shared by approve/deny/steer) now captures the runID each call was issued for and skips writing connectionError when currentRunID no longer matches it on completion -- the same captured-runID guard cancel()/answer() already use, so a Task that resolves after reset() has moved the session onto a different (or no) run cannot resurrect state for the abandoned one. Test runner output (expected: all passing): Test run with 276 tests in 52 suites passed after 5.687 seconds. (12/12 in "RunSession control acknowledgements", including the new regression test; full suite unaffected.) Files changed: Sources/GoCodeUI/RunSession.swift
This was referenced Jul 31, 2026
Owner
Author
|
@codex review Please review exact head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
go-code/macapp/Sources/GoCodeUI/ChatView.swift
Lines 42 to 44 in 575b670
Reset answers when the pending prompt changes
If the server emits a second run.waiting_for_user before the first answer request completes, the new ownership check correctly preserves that newer prompt, but SwiftUI retains AskUserView and its @State answers because the view has no identity tied to prompt.callID. When both prompts contain a question with the same index and text, the retained dictionary satisfies the new prompt and can submit the previous answer; key this view by the call ID or explicitly clear its state when the call changes.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review Please review the new final exact head |
Owner
Author
|
Hosted verification is now terminal on exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01268cf33c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
Final review repairs are now at exact head @codex review |
Owner
Author
|
Exact-head hosted checks are now all green at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa92d20c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
The two unresolved accounting threads are repaired at exact head @codex review |
Owner
Author
|
Hosted checks are terminal and all green at exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7361769b0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
Four final review repairs are now at exact head @codex review |
Owner
Author
|
Hosted checks are terminal and all green at exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f76a74a35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
Latest exact-head repairs are at @codex review |
Owner
Author
|
Exact-head hosted verification is terminal and all green at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec0a6654d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The conversation stream can deliver background/callback events | ||
| // from other runs while `startRun` is in flight. Only the id | ||
| // returned by that request may end this reservation. | ||
| return false |
There was a problem hiding this comment.
When submitting a follow-up on an existing conversation, harnessd dispatches the run before the POST /v1/runs response is written, so the conversation SSE can deliver even the terminal event while this branch returns false. Fresh evidence at the repaired head is that RunSession.apply nevertheless inserts the event ID into seenEventIDs before calling Transcript.apply; after bindAccountingRun, the per-run replay is then discarded as duplicate. A fast run can consequently leave the transcript permanently .queued with zero accounting, so reserved events need to be buffered/replayed or not marked seen until their identity is bound.
Useful? React with 👍 / 👎.
| // waiting for the new run id. | ||
| previousAccountingRunID = accountingRunID | ||
| accountingRunID = nil | ||
| awaitingAccountingRun = true |
There was a problem hiding this comment.
If startRun throws before bindAccountingRun—including when the server accepted the run but the client lost its response—this flag remains set after markFailed() handles the local error. Every later conversation-stream run then has its queued, started, terminal, and accounting events rejected by prepareAccounting, so scheduled callbacks or other external runs cannot update busy state or usage until the operator resets or successfully submits again; the startup failure path needs to abandon or otherwise resolve this reservation.
Useful? React with 👍 / 👎.
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.
Summary
Implements epic #991's 8 child slices on one branch, test-first per slice, followed by an adversarial review and production-review repair:
rewind_refusedbecomes a real, actionable state with its own, more severe force confirmation.Closes #992
Closes #993
Closes #994
Closes #995
Closes #996
Closes #997
Closes #998
Closes #999
Related: #991
Production-review repair
Exact repaired head:
cec0a6654da96c7ef3ab812c489338f72c4fd6db.The repair merged current
mainatb3afc7ec487c60762a91a1219ceb92c523ef0e78and preserves #1008 persisted/live replay deduplication and #1028 failed/cancelled terminal reconciliation. It adds:callIDso prior answers cannot cross prompts;External scheduled-run control identity remains #1007 and is intentionally not implemented here; external conversation-stream activity still contributes to the #995 busy guard.
Test evidence
swift build --package-path macapppassed;swift test --package-path macapppassed 316 tests / 55 suites; strict recursive Swift format lint passed.RunSessionLiveTestsreproduced the hosted zero-usage failure locally, then passed after the terminal/replay accounting repair.go test ./internal/server ./internal/harness ./internal/storepassed../scripts/test-regression.shrun passed at 85.6% coverage with zero uncovered functions. The first hosted race attempt and 10/10 targeted sandboxed repetitions exposed the known current-mainTestWorktreeContainment_ToolCwdIsWorktreecleanup race owned by [Bug]: Worktree containment test races workspace teardown after tool completion #1039 / green PR test: synchronize worktree containment cleanup #1041 (bd0682c4); the exact repaired-head hosted rerun is fully green. PR feat(macapp): GUI correctness, safety, and accessibility hardening (epic #991) #1021 deliberately does not duplicate that unrelated Go test fix. Safe promotion order is test: synchronize worktree containment cleanup #1041 first, then refresh and recheck feat(macapp): GUI correctness, safety, and accessibility hardening (epic #991) #1021.Review evidence
1f2444b2480b5832139318e4fa034f4240d92b8dconfirmed the remaining async/control/process gaps; the repair above closes those source and automated-test findings.cec0a665.docs/residual-review-findings/feat-macapp-gui-hardening.mdand fix(macapp): setCost failure status erased by its own reload + deferred manual smokes from #991 #1020.Pending live proof
Installed-app macOS smokes and the separate Settings-specific
setCostroot-cause investigation remain pending. Exact-head hosted checks are green, but safe stacking still puts the already-owned #1039 / PR #1041 cleanup repair ahead of #1021. None of the pending native/Settings proof is claimed or waived, and this PR must not merge until those follow-ups are reconciled.Deviations
ChatView.swift,ProjectSession.swift, andSessionsView.swift, so preserving the already-existing child commit stack in one repaired PR is safer than duplicating or splitting code now.Plan:
docs/plans/2026-07-30-001-feat-macapp-gui-hardening-plan.mdImpact map:
docs/plans/2026-07-31-pr-1021-gui-hardening-repair-impact-map.md🤖 Generated with Compound Engineering