Skip to content

Make Augment keepalive stop idempotent#1262

Draft
ProspectOre wants to merge 3 commits into
steipete:mainfrom
ProspectOre:codex/augment-keepalive-stop-idempotent
Draft

Make Augment keepalive stop idempotent#1262
ProspectOre wants to merge 3 commits into
steipete:mainfrom
ProspectOre:codex/augment-keepalive-stop-idempotent

Conversation

@ProspectOre
Copy link
Copy Markdown
Contributor

@ProspectOre ProspectOre commented Jun 1, 2026

Summary

Skip Augment keepalive stop work when no keepalive is active, avoiding repeated stop logs for already-stopped runtimes.

Context

Part of the UI hang/lag cleanup set: this removes redundant disabled-provider runtime work and log churn from repeated stop paths.

Validation

  • swift test --filter AugmentProviderRuntimeTests
  • swift test --filter AugmentStatusProbeTests
  • git diff --check

Runtime Proof

Fixed-branch test proof at d0b2082d:

$ swift test --filter AugmentProviderRuntimeTests
Test Case '-[CodexBarTests.AugmentProviderRuntimeTests test_disabledStopDoesNotLogWithoutKeepalive]' passed (0.165 seconds).
Executed 1 test, with 0 failures

Negative proof against main using a temporary worktree with only the new test file applied:

$ swift test --filter AugmentProviderRuntimeTests
... XCTAssertFalse failed - Disabled stop calls should not emit stopped logs when no keepalive exists:

[2026-06-01T06:49:15.230Z] [INFO] com.steipete.codexbar.augment: Augment keepalive stopped (provider disabled)
[2026-06-01T06:49:15.230Z] [INFO] com.steipete.codexbar.augment: Augment keepalive stopped (provider disabled)
Executed 1 test, with 1 failure

Live app runtime proof from a freshly packaged debug app at d0b2082d, captured June 1, 2026 at 00:12 PDT with unified logging. App startup exercises the disabled-provider runtime path (stop followed by settingsDidChange). The log is redacted to startup/provider state only:

$ CODEXBAR_SIGNING=adhoc CODEXBAR_ALLOW_LLDB=1 Scripts/package_app.sh debug
Created /Users/alec/Dev/CodexBar/CodexBar.app

$ log stream --info --style compact --predicate 'process == "CodexBar" && subsystem == "com.steipete.codexbar"'
2026-06-01 00:12:49.251 I CodexBar[...] [com.steipete.codexbar:app] CodexBar starting (git=d0b2082d version=0.32.3)
2026-06-01 00:12:49.565 I CodexBar[...] [com.steipete.codexbar:providers] Provider enablement at startup (enabled=claude,codex states=...,augment=0,...)

$ rg -c "Augment keepalive stopped" oslog.txt
0

Compatibility check already run for this PR:

$ swift test --filter AugmentStatusProbeTests
Executed 17 tests, with 0 failures

Also ran git diff --check; it passed with no output. The runtime proof does not include account, token, cookie, or host data.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 4:16 AM ET / 08:16 UTC.

Summary
The branch adds a nil-keepalive guard to AugmentProviderRuntime.stopKeepalive and a regression XCTest for repeated disabled stop/settingsDidChange calls.

Reproducibility: yes. Source inspection shows current main can call Augment stop while disabled and log a stopped message without an active keepalive; the PR body also includes negative test output against main.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Confirm make check or the repository’s equivalent format/lint validation before merge if maintainers require it.

Risk before merge

  • [P1] This read-only review did not run swift test or make check; the PR body reports focused tests and git diff --check, but AGENTS.md asks for make check after code changes.
  • [P1] The PR is still draft with unstable check state, so maintainers should let GitHub checks and any required repository validation gate the final merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land the nil-keepalive guard with the focused regression test after draft/check completion and any required make check validation are confirmed.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair is needed; the remaining action is normal maintainer review, draft/check completion, and repository validation confirmation.

Security
Cleared: The diff only changes an in-process runtime guard and adds an XCTest, with no dependency, CI, signing, secret, or supply-chain surface changes.

Review details

Best possible solution:

Land the nil-keepalive guard with the focused regression test after draft/check completion and any required make check validation are confirmed.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main can call Augment stop while disabled and log a stopped message without an active keepalive; the PR body also includes negative test output against main.

Is this the best way to solve the issue?

Yes. Returning early only when no keepalive exists is the narrowest maintainable fix, and the added test covers the repeated disabled stop plus settingsDidChange path.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 4756ba06bf42.

Label changes

Label justifications:

  • P3: This is a low-risk provider-runtime idempotency/log-churn fix with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body includes redacted live app unified-log output from a freshly packaged debug app showing the disabled Augment startup path produced zero stopped logs, plus fixed and negative test output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted live app unified-log output from a freshly packaged debug app showing the disabled Augment startup path produced zero stopped logs, plus fixed and negative test output.
Evidence reviewed

What I checked:

  • Current main repeats the disabled-runtime stop path: UsageStore.updateProviderRuntimes calls stop for disabled providers and then settingsDidChange, which makes an unconditional stop log observable for disabled Augment runtimes. (Sources/CodexBar/UsageStore.swift:516, 4756ba06bf42)
  • Current Augment stop logs even with no keepalive: Current main calls self.keepalive?.stop(), clears keepalive, and logs "Augment keepalive stopped" without first checking that a keepalive was active. (Sources/CodexBar/Providers/Augment/AugmentProviderRuntime.swift:84, 4756ba06bf42)
  • PR diff is a narrow guard plus regression test: The PR changes stopKeepalive to return when self.keepalive is nil and adds AugmentProviderRuntimeTests that assert repeated disabled stop/settingsDidChange calls do not emit the stopped log. (Sources/CodexBar/Providers/Augment/AugmentProviderRuntime.swift:86, d0b2082d7e87)
  • Contributor proof includes real runtime logs: The PR body includes fixed-branch test output, negative test proof against main, and redacted live app unified-log proof from a freshly packaged debug app showing zero Augment keepalive stopped logs on disabled-provider startup. (d0b2082d7e87)
  • Repository policy was read and applied: AGENTS.md is 45 lines and was read fully; it favors focused parser/provider tests and requires make check after code changes, which informs the residual validation note. (AGENTS.md:1, 4756ba06bf42)
  • Feature-history provenance: History points to steipete for the provider runtime split/current runtime hook and bcharleson for the original Augment keepalive implementation and recent adjacent Augment work. (Sources/CodexBar/Providers/Augment/AugmentProviderRuntime.swift:1, a2fb3fb1c5ea)

Likely related people:

  • steipete: Commit a2fb3fb split provider settings/runtime hooks and introduced AugmentProviderRuntime; recent UsageStore/runtime history also has multiple steipete changes. (role: runtime refactor and recent area contributor; confidence: high; commits: a2fb3fb1c5ea, 4c9e6a87009b, 3f419060f875; files: Sources/CodexBar/Providers/Augment/AugmentProviderRuntime.swift, Sources/CodexBar/UsageStore.swift)
  • bcharleson: Commit 87ace28 introduced AugmentSessionKeepalive, and 4a2ef3a recently touched adjacent Augment runtime/keepalive code. (role: original Augment keepalive contributor and adjacent provider contributor; confidence: high; commits: 87ace28a6c80, daeab0c8e724, 4a2ef3ae18f6; files: Sources/CodexBarCore/Providers/Augment/AugmentSessionKeepalive.swift, Sources/CodexBar/Providers/Augment/AugmentProviderRuntime.swift, Tests/CodexBarTests/AugmentStatusProbeTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jun 1, 2026
@ProspectOre
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@ProspectOre
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 1, 2026
@ProspectOre
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant