feat(bridge): GitHub as a coordination substrate for multiple agentflare instances - #379
Merged
Conversation
…, remove overflow sentinel A cede retired all of an owner's claims unconditionally, so an instance that crashed, ceded defensively, restarted and re-claimed was locked out of that issue permanently. Retire only claims preceding the cede. A done marker from any owner ended the issue, so a single stray or misdirected done was a standing denial-of-service on it. Require the done's owner to have posted an earlier claim. latest_ts_for returned i64::MIN for an unknown owner, which panicked on subtract-with-overflow under debug overflow checks. Return Option and keep a saturating_sub at the call site for corrupt markers carrying ts == i64::MIN.
…e state::first_in_group, add metadata edge-case tests - config.rs: doc-comment max_claims explaining 0 is legitimate drain mode (stop claiming new work, keep re-verifying/exporting held issues) and is deliberately not floored like interval_secs; add a regression test. - config.rs: trim AGENTFLARE_BRIDGE_INSTANCE_ID before the emptiness check so a whitespace-only value falls back to claims::owner_id, consistent with the truthy() helper. - items.rs: state_id_for_group now delegates to the backend's state::first_in_group (one indexed query) instead of scanning list_by_project client-side; signature unchanged. - items.rs: add tests for with_last_hash on non-object JSON metadata (array/number/string/null/bool) and last_hash on non-string github_last_hash values (number/object/null/array).
…apacity Both bugs share one root cause: nothing distinguished "actively held" from "locally cancelled but still linked to the issue". Cancelling an item moves it to the cancelled state group but never sets completed_at (only the started/completed groups touch timestamps), so completed_at alone can't tell the two apart. Add items::is_active(), which checks the item's actual state group instead, and use it in both places that were fooled by the missing signal: - tick.rs step 1 now skips items already in the cancelled/completed group, so a ceded item is not re-ceded (and re-spamming the issue with a "ceding this" comment) on every subsequent poll. - items_tracked() now filters on state group rather than completed_at, so ceded items no longer permanently consume claim capacity. Also removes the vestigial TickReport.imported field, left over from an earlier push-routing design that no longer applies under the claim-only pull model.
Marker liveness is judged per owner id, and the bridge derived its id from claims::owner_id -- whose instance half is AGENTFLARE_SESSION or the pid. Every daemon restart therefore became a brand-new owner: i_hold went false against markers the previous process had written, so the bridge ceded and cancelled every item it was still actually working, and minted a fresh claimed:<agent>:<pid> label per lifetime. The discriminator now lives in ~/.agentflare/bridge-instance-id and is created with create_new, so two processes starting at once converge on one id instead of clobbering each other.
…n work The spec promised a heartbeat; the code never had one. claim.rs judges an owner live by its newest marker, and the only thing that ever wrote a marker was export_if_dirty -- which fires solely on a content-hash change. An issue being worked rather than edited therefore went quiet, aged past the TTL, and the instance ceded its own live work to nobody, cancelled the local item, and could never reclaim it. Pure wall-clock timer: one instance, no adversary. Two halves, both required: - Every tick we still hold an issue now refreshes the ledger lease, and refreshes the GitHub marker once it is older than half the TTL. The remote refresh EDITS the comment already carrying the claim rather than posting a new one, so the comment id -- which resolve_holder orders by and is_ceded compares against -- is unchanged, and the issue does not collect a bookkeeping comment every 15 minutes. It also rewrites item= with the real item id, unknown at claim time. - A cede is no longer terminal. Step 3 skipped on the mere existence of a linked item, so the cancelled row a cede leaves behind locked this instance out of that issue forever. Only cancelled items are re-adoptable -- completed ones still owe their issue a done export -- and reclaiming re-adopts the existing row instead of creating a second one that find_by_issue would never return. Issues ceded earlier in the same tick are left until the next one.
…phan claim I1: `take(headroom)` bounded claim ATTEMPTS, not successes. A queue whose first few entries were already tracked or held by someone else consumed the whole allowance and this instance claimed nothing, every tick, for as long as the queue head stayed put — while free issues sat right below it. The loop now walks the whole queue and decrements a counter only on a successful claim. I2: step 2 had no guard, so a ceded item — cancelled but still linked to its issue — reached export_if_dirty and posted "Progress from ..." on an issue another instance now owns. It was not a rare drift either: labels feed the content hash and the winner adds its own `claimed:` label, so the hash changes by construction. The guard is `is_ceded`, deliberately not `!is_active`: a completed item is also inactive but still owes its issue the `done` export and the close that follows. I3: try_claim bailed on `resolve_holder(...).is_some()` without checking WHO held it. When GitHub said we held an issue but we had no local item, we sat out a full TTL waiting for our own marker to expire — reachable from a Transport blip between posting the claim and re-reading comments, from state_id_for_group returning None after we won, and from the local db loss marker.rs advertises as recoverable. Our own claim is now adopted instead, through the same record_claim path an ordinary win takes.
…ailures I4: every latch was a discarded `let _ =`. Both the export hash and the cede cancellation are what stop the next tick redoing the work, so losing one means posting the identical comment every interval_secs forever -- 1440 a day at the defaults, and `database is locked` from the dashboard or MCP server writing the same file makes it reachable. Both now write the local latch FIRST and gate the GitHub write on it, so nothing can be posted that we then fail to record. A failed remote write rolls the latch back, so the work is retried next tick instead of being suppressed until the content happens to change. Failures at every step are logged rather than dropped. I5: the likeliest startup failures were the silent ones -- current_dir, RepoId::resolve_from_remote and resolve_project all vanished through bare `.ok()?`. Each now says what went wrong. resolve_from_remote derives the repo from cwd and neither the launchd plist nor the systemd unit sets a working directory, so AGENTFLARE_BRIDGE_REPO=owner/repo now overrides it and the cwd-derived path says so when it fails. Soft errors are no longer swallowed whole either: run_once reports the one that ended the tick, and the runner logs a CHANGE in that state. A rate limit still stays quiet across retries, but a Forbidden -- a permanently dead bridge -- now says so once, and says so again when it recovers.
two_instance.rs proved nothing. Its five tests called i_hold/resolve_holder on hand-built comment vectors, and `a ^ b` is tautological there: resolve_holder returns Option<Holder>, so at most one owner satisfies it by the return type, not by anything the protocol does. Every assertion passed while the tick orchestration was ceding its own live work every TTL, because none of the bugs were in the arbitration functions -- they were in what tick.rs did with the answers. It now builds two instances with two backends, two ledgers and two owner ids, sharing nothing but a mock GitHub whose comment list, labels and open/closed state are real mutable state both of them read and write. Every test drives run_once and asserts on the two private databases and the shared issue. Covered: two instances converge on one holder; a stalled holder loses the issue after the TTL and cedes on its next tick; a WORKING instance keeps its claim across four TTLs of ticks while the other never takes it (the C1 regression -- this fails outright without the heartbeat); a finished issue closes and leaves the queue; and a ceded issue can be picked up again by the instance that ceded it. Supporting change: MockServer grows start_with(handler), since a comment list two clients both write to cannot be expressed as a fixed response script. Over-requesting the fixed-queue form now returns a 500 naming the method and path instead of leaving the client blocked on a read that never comes. That new test immediately found a bug none of the 14 unit tests could: tick::cede builds its marker with hash: "", which rendered as `hash=`, and parse fails closed on an empty value -- so EVERY cede the bridge has ever posted was unparseable. No other instance could see a claim being given up, and is_ceded never retired the ceding owner's claim. The unit tests missed it because their fixtures used a non-empty hash the production code never wrote. render now substitutes "-" for an empty field, at that single choke point so a future field cannot reintroduce it.
M1: the 14 `#[allow(dead_code)] // consumer arrives in a later bridge task` markers are stale now the daemon calls the tick, so they are gone. They were hiding one genuinely dead function: issue_body_with_marker, left over from a body-marker design that was abandoned for comment-only markers. Deleted, with a note saying why markers live in comments. list_filtered's `since` doc claimed it "keeps each bridge poll cheap" while the bridge only ever passes None; the doc now says why None is correct — a since-filtered listing hides exactly the untouched issues that are most claimable. M2: claim markers no longer say `item=pending` forever. The marker is posted before the item exists, so it cannot name it; issues::comment now returns the new comment id and record_claim rewrites the marker with the real item id as soon as there is one. M3: `claimed:<instance>` labels come back off on cede and on completion. Previously every issue accumulated one label per instance that ever touched it, and the issue list showed work claimed by nobody. Adds issues::remove_label, which treats a 404 as success — the desired end state holds either way. M4: dropped the debug_assert_eq! inside the poll loop. A panic there kills the thread with no restart and no notice (the JoinHandle is discarded), and the invariant it checked — that the item we just created carries the issue number we just passed — cannot fail. M5: spawn_if_enabled no longer does startup work on the caller's thread. It shelled out to `git remote` and potentially to `gh auth token`, delaying the daemon binding its dashboard port; all of it now happens on the spawned thread. M6: the bridge no longer holds the shared backend-db mutex across network I/O. It ran a whole tick's worth of GitHub round trips inside with_backend_db, so every MCP tool call and dashboard read blocked behind the bridge's network latency. It opens its own connection to the same file instead.
Both found by running the bridge against a real GitHub repo, and both cost one junk comment on every issue it ever claims. The `claimed:<instance>` label fed the content hash. The hash exists to notice that an issue's CONTENT drifted, and that label is not content -- it is something the bridge itself wrote moments earlier. So the sequence was: claim, add `claimed:<us>`, and on the very next tick the hash had "changed", so post "Progress from ..." reporting our own side effect. `content_labels` now filters the prefix out, and a label a HUMAN adds still counts. A freshly claimed item had no stored export hash, so the first tick after a claim always exported -- announcing an item whose name and description had been copied from that same issue seconds before. `record_claim` now seeds the latch with what it imported. A real local edit still changes the hash and still exports. The latch value is now one function, `export_hash`, because the seeding side and the comparing side have to agree exactly; computing it in two places is what would silently bring the spurious comment back. Also fixes a race in the persisted instance id from 2d72b9d, caught by its own concurrency test once that test was made to run 25 rounds instead of one: `create_new` publishes the file the instant it is created, which is before the winner has written any bytes, so a losing process could read it empty and conclude the id was unusable. In production that meant falling back to a pid-derived id -- reintroducing the mass-cede C2 exists to prevent -- and logging a warning on a perfectly healthy machine. The loser now waits for the winner to finish, and only reports an error if nobody ever does.
The manual verification step was a prose checklist nobody would repeat. It is
now two ignored tests, gated on AGENTFLARE_LIVE_GITHUB_REPO naming a
throwaway repo, that drive the real run_once against real GitHub:
AGENTFLARE_LIVE_GITHUB_REPO=owner/scratch \
cargo test --bin agentflare live_github -- --ignored --nocapture
They cover the questions no mock can answer, because the answer lives in
GitHub's behavior rather than ours:
- the marker really is INVISIBLE. Asserted against body_html -- what GitHub
RENDERS -- not the body we wrote. Every other test in the suite would still
pass if GitHub stopped hiding HTML comments and the bridge started spraying
machine noise across every issue it touched.
- editing a comment really does leave its id untouched, which is what
resolve_holder's lowest-id-wins ordering depends on.
- a `claimed:` label with colons in it really is addressable on the label
delete endpoint.
- an instance holding an issue across 3 TTLs of ticks still holds it, using
exactly ONE comment (C1, end to end).
- a cede written by one instance really does parse for another one -- the
empty-hash bug, proven against real GitHub rather than a fixture.
Supporting changes: Client::request_with_accept, since body_html needs a
different Accept media type; and test-gated issues::delete_comment /
issues::reopen so a scratch repo can be reset between runs. Both are cfg(test)
-- the bridge itself only ever appends and edits, so its history stays
auditable, and it never reopens what it closed.
Verified against a real private scratch repo: claim -> heartbeat-by-edit ->
done -> close -> label removal, plus the daemon path (AGENTFLARE_BRIDGE_REPO
from a cwd that is not a repo) and a restart that correctly did NOT cede its
in-flight work. Deliberately leaves its comments and labels behind; they are
the evidence you eyeball in the browser afterwards.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opt-in GitHub Coordination Bridge. The bridge arbitrates issue claims with markers and leases, synchronizes local items with GitHub, runs polling in a background thread, and validates coordination through mock and live integration tests. ChangesGitHub Coordination Bridge
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant BridgeRunner
participant GitHub
participant ClaimLedger
participant ItemDatabase
Dashboard->>BridgeRunner: spawn_if_enabled()
BridgeRunner->>GitHub: resolve repository, project, and queue
BridgeRunner->>ClaimLedger: open claim ledger
BridgeRunner->>BridgeRunner: run polling interval
BridgeRunner->>GitHub: list queued issues and comments
BridgeRunner->>ClaimLedger: acquire or renew claim
BridgeRunner->>ItemDatabase: create or update linked item
BridgeRunner->>GitHub: write markers, labels, and exports
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/github/bridge/tick.rs (2)
88-117: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a rate budget for per-issue comment reads.
Step 1 calls
comment_pairsfor every queued issue that has an active local item. Step 3 calls it again for each claim candidate.issues::list_commentsis paginated, so one tick costs at least1 + N + Mrequests. Atinterval_secs=60and a queue of 50 issues, this can consume a large share of the hourly REST budget and causeRateLimitedticks.Two options that need no protocol change:
- Pass
sincetolist_commentsfor the re-verify path, since only recent markers change liveness.- Track the last-seen comment id per issue and skip the re-read when the issue's
updated_atdid not move.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/tick.rs` around lines 88 - 117, Reduce redundant per-issue comment reads in the tick flow around comment_pairs, especially the re-verification loop and claim-candidate processing. Reuse cached comments or skip reads when an issue has not changed, or pass an appropriate since boundary so only recent claim markers are fetched; preserve holder resolution, heartbeat, cede, and claim behavior while keeping request volume within the rate budget.
417-422: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the
claimed:label write best-effort.
add_labelspropagates with?. At this point the item, the ledger row, the marker rewrite, and the export latch are all committed. A label failure therefore discards thetruereturn, keeps the claim out ofTickReport.claimed, and ends the whole tick before step 3 reaches the remaining issues. The label is bookkeeping only; the removal path at Line 490 already treats it that way.♻️ Proposed change
- issues::add_labels( + if let Err(e) = issues::add_labels( &ctx.client, &ctx.repo, issue.number, &[format!("{CLAIMED_LABEL_PREFIX}{}", ctx.config.instance_id)], - )?; + ) { + eprintln!( + "github bridge: could not add the claimed label to #{}: {e}", + issue.number + ); + } Ok(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/tick.rs` around lines 417 - 422, In the claim-handling flow around issues::add_labels, make writing the claimed label best-effort by handling its error locally instead of propagating it with ?. Preserve the committed claim result and continue processing remaining issues, matching the removal path’s bookkeeping behavior..superpowers/sdd/progress.md (1)
130-132: 📐 Maintainability & Code Quality | 🔵 TrivialOpen item recorded: manual GitHub UI verification.
Lines 130-132 record that the marker's invisibility in the GitHub web UI is still unverified. I can open a tracking issue for this checklist item so it does not stay only in this ledger. Do you want me to open it?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.superpowers/sdd/progress.md around lines 130 - 132, Track the manual GitHub UI verification item from the checklist in PLAN-corrected-v2.md as a separate issue, explicitly covering verification that the marker renders invisibly in GitHub’s web UI using a human and scratch repository. Update the progress entry to reference the tracking issue once created.src/github/bridge/runner.rs (1)
142-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd backoff for consecutive failed ticks.
The loop sleeps a constant
interval_secsafter every outcome. If the tick ends withRateLimited, the next tick issues the same request set one interval later and gets limited again. The same applies to a hardTransporterror during an outage. At the default interval that is 1440 wasted request sets a day, and it delays the recovery of the secondary rate limit.Track consecutive failures and grow the sleep up to a cap. If the response carries a rate-limit reset time, prefer sleeping until then.
⏱️ Proposed change
let mut last_soft: Option<String> = None; + let mut failures: u32 = 0; loop { let now = crate::claims::now(); + let mut failed = false; match run_once(&ctx, &conn, now) { Ok(report) => { if !report.claimed.is_empty() || !report.ceded.is_empty() { eprintln!( "github bridge: claimed {:?} ceded {:?}", report.claimed, report.ceded ); } + failed = report.soft_error.is_some(); if report.soft_error != last_soft { match &report.soft_error { Some(e) => eprintln!("github bridge: ticks are ending early: {e}"), None => eprintln!("github bridge: recovered; ticks completing again"), } last_soft = report.soft_error; } } - Err(e) => eprintln!("github bridge: tick failed: {e}"), + Err(e) => { + failed = true; + eprintln!("github bridge: tick failed: {e}"); + } } - std::thread::sleep(interval); + failures = if failed { failures.saturating_add(1) } else { 0 }; + // Cap the backoff so a recovered bridge resumes promptly. + let factor = 1u64 << failures.min(5); + std::thread::sleep(interval.saturating_mul(factor as u32)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/runner.rs` around lines 142 - 163, Update the runner loop around run_once to track consecutive RateLimited and hard Transport failures, increasing the delay between ticks with a bounded backoff cap while resetting the failure count after a successful tick. When a rate-limit reset time is present, use the duration until that reset instead of the calculated backoff; retain the existing interval for normal successful ticks and continue sleeping after every outcome.src/github/bridge/tests/live_github.rs (1)
299-310: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reducing the tick count in the live sustain loop.
The loop runs about 90 ticks. Each tick calls the live GitHub API several times (queue listing, comment listing, and periodic marker edits). One run therefore issues a few hundred real requests, which can hit secondary rate limits and make the manual verification unreliable. A larger step, for example
t += TTL / 4, still crosses three TTLs and several heartbeat windows with far fewer requests.♻️ Proposed change
let mut t = hb_at; while t < t0 + TTL * 3 { - t += 60; + // Big enough steps to keep the live request count sane, small enough + // to land several heartbeat windows inside each TTL. + t += TTL / 4; l.tick(t);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/tests/live_github.rs` around lines 299 - 310, Reduce the iteration count in the live sustain loop around the `while t < t0 + TTL * 3` block by advancing `t` in larger intervals such as `TTL / 4` instead of fixed 60-second steps. Preserve coverage across three TTLs and the existing heartbeat/active-work assertions while minimizing live GitHub API requests.src/github/bridge/config.rs (2)
180-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim
queue_labelbefore the empty check.
from_envtrimsAGENTFLARE_BRIDGE_INSTANCE_IDbutfrom_valuesdoes not trimqueue_label. A value of" "passes the!s.is_empty()filter. The bridge then filters the queue on a whitespace label, matches no issues, and does nothing without saying why.♻️ Proposed change
queue_label: queue_label + .map(str::trim) .filter(|s| !s.is_empty()) .unwrap_or(DEFAULT_QUEUE_LABEL) .to_string(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/config.rs` around lines 180 - 183, Update the queue_label handling in from_values to trim the value before checking whether it is empty, so whitespace-only labels use DEFAULT_QUEUE_LABEL and non-empty labels are stored without surrounding whitespace.
87-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the file if the write fails.
create_newpublishes the file before any bytes reach it. Ifwrite_allorsync_allthen fails, the file stays on disk and empty. Every later start takes theAlreadyExistsbranch, waits the fullINSTANCE_ID_WRITE_WAIT, and falls back to a pid-derived id. Recovery needs a manual delete.Deleting the empty file on write failure makes the next start re-mint automatically.
♻️ Proposed change
Ok(mut f) => { - f.write_all(fresh.as_bytes())?; - f.sync_all()?; - Ok(fresh) + // Do not leave a published-but-empty file behind: every later + // start would wait out INSTANCE_ID_WRITE_WAIT and then fall back + // to a pid-derived id until a human deletes it. + if let Err(e) = f.write_all(fresh.as_bytes()).and_then(|()| f.sync_all()) { + drop(f); + let _ = std::fs::remove_file(path); + return Err(e); + } + Ok(fresh) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/config.rs` around lines 87 - 91, Update the file-creation flow around the create_new result handling so any write_all or sync_all failure removes the newly created file before propagating the original error. Preserve successful writes and ensure cleanup is attempted for failures after create_new publishes the file, including partial writes.src/github/bridge/items.rs (2)
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for
is_activeandis_ceded.The DB-backed tests cover
find_by_issueandstate_id_for_group, but notis_activeoris_ceded. Both encode a deliberate asymmetry:completedis not active and not ceded, and unresolvable state fails open foris_activeand closed foris_ceded. A direct test over the seededstarted,completed, andcancelledgroups pins that contract in the file that owns it, instead of relying ontick.rscoverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/items.rs` around lines 255 - 261, Add direct DB-backed tests for is_active and is_ceded using the seeded started, completed, cancelled, and an unresolvable state. Assert completed is neither active nor ceded, and assert unresolvable states fail open for is_active but closed for is_ceded, alongside the expected started/cancelled behavior.
19-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the project item list per tick.
find_by_issueloads and materializes every item in the project on each call.tick::run_innercalls it up to three times per queued issue, so one tick performsO(queue_size × project_items)row conversions. The doc comment justifies a single scan, but not one scan per issue per step.Building one
HashMap<String, Item>keyed byexternal_idat the start of a tick keeps the same semantics and removes the repeated scans.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/bridge/items.rs` around lines 19 - 28, Cache project items once per tick instead of reloading them in each find_by_issue call. Update tick::run_inner to build a HashMap<String, Item> for the relevant project(s) at tick start, keyed by external_id, and have find_by_issue reuse that cache while preserving the EXTERNAL_SOURCE and issue-number matching semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.superpowers/sdd/progress.md:
- Line 3: Remove the machine-local absolute path from the “Local plan” entry in
progress.md, leaving the existing session-scoped scratchpad guidance and durable
PLAN-corrected-v2.md reference unchanged.
In `@src/github/bridge/claim.rs`:
- Around line 48-50: Update the marker-validity filter around latest_ts_for so
future timestamps are not accepted indefinitely: require timestamps to be no
later than now plus ttl_secs while retaining the existing TTL check for
timestamps at or before now. Apply the same upper-bound handling in
resolve_holder for parsed comment timestamps, ensuring a future-dated marker
expires by now + ttl_secs.
- Around line 27-42: Update the issue_done logic in the claim resolution flow so
a Done marker only blocks resolution when no later Claim exists; a Claim with a
higher comment id must supersede the Done, regardless of owner. Preserve the
existing requirement that Done has a prior matching owner claim, and add
coverage for claim(1,a), done(2,a), claim(3,b) resolving to b.
In `@src/github/bridge/marker.rs`:
- Around line 62-77: Validate AGENTFLARE_BRIDGE_INSTANCE_ID in
BridgeConfig::from_env, rejecting whitespace, “=” or “/” and reporting the error
before proceeding. Update marker::field to replace whitespace and “=” in marker
values, and update the claimed label construction in issues.rs to encode the
instance id as a path segment so “/” is escaped; apply these changes at
src/github/bridge/marker.rs:62-77 and src/github/issues.rs:205-224.
In `@src/github/bridge/tick.rs`:
- Around line 329-331: Update the missing-“started” state branch in record_claim
to log the condition before returning Ok(false), matching the logging behavior
used by cede for the mirror-image state lookup failure. Preserve the existing
return value and claim flow.
- Around line 587-607: Separate the comment export from the close operation
around issues::comment and issues::close: persist the export latch once the
comment succeeds, then treat close failures as independent retryable errors
without rolling back the comment hash. Add a retry path for completed items
whose issue state is still open, so an already-exported item attempts
issues::close again without reposting the completion comment.
- Around line 631-645: Update store_hash and the rollback path around
with_last_hash to avoid overwriting concurrent metadata changes: re-read the
current item metadata and atomically merge only the export-hash field before
each write, or store the hash in a bridge-owned column. Preserve unrelated MCP
item(update) metadata changes in both success and rollback flows.
---
Nitpick comments:
In @.superpowers/sdd/progress.md:
- Around line 130-132: Track the manual GitHub UI verification item from the
checklist in PLAN-corrected-v2.md as a separate issue, explicitly covering
verification that the marker renders invisibly in GitHub’s web UI using a human
and scratch repository. Update the progress entry to reference the tracking
issue once created.
In `@src/github/bridge/config.rs`:
- Around line 180-183: Update the queue_label handling in from_values to trim
the value before checking whether it is empty, so whitespace-only labels use
DEFAULT_QUEUE_LABEL and non-empty labels are stored without surrounding
whitespace.
- Around line 87-91: Update the file-creation flow around the create_new result
handling so any write_all or sync_all failure removes the newly created file
before propagating the original error. Preserve successful writes and ensure
cleanup is attempted for failures after create_new publishes the file, including
partial writes.
In `@src/github/bridge/items.rs`:
- Around line 255-261: Add direct DB-backed tests for is_active and is_ceded
using the seeded started, completed, cancelled, and an unresolvable state.
Assert completed is neither active nor ceded, and assert unresolvable states
fail open for is_active but closed for is_ceded, alongside the expected
started/cancelled behavior.
- Around line 19-28: Cache project items once per tick instead of reloading them
in each find_by_issue call. Update tick::run_inner to build a HashMap<String,
Item> for the relevant project(s) at tick start, keyed by external_id, and have
find_by_issue reuse that cache while preserving the EXTERNAL_SOURCE and
issue-number matching semantics.
In `@src/github/bridge/runner.rs`:
- Around line 142-163: Update the runner loop around run_once to track
consecutive RateLimited and hard Transport failures, increasing the delay
between ticks with a bounded backoff cap while resetting the failure count after
a successful tick. When a rate-limit reset time is present, use the duration
until that reset instead of the calculated backoff; retain the existing interval
for normal successful ticks and continue sleeping after every outcome.
In `@src/github/bridge/tests/live_github.rs`:
- Around line 299-310: Reduce the iteration count in the live sustain loop
around the `while t < t0 + TTL * 3` block by advancing `t` in larger intervals
such as `TTL / 4` instead of fixed 60-second steps. Preserve coverage across
three TTLs and the existing heartbeat/active-work assertions while minimizing
live GitHub API requests.
In `@src/github/bridge/tick.rs`:
- Around line 88-117: Reduce redundant per-issue comment reads in the tick flow
around comment_pairs, especially the re-verification loop and claim-candidate
processing. Reuse cached comments or skip reads when an issue has not changed,
or pass an appropriate since boundary so only recent claim markers are fetched;
preserve holder resolution, heartbeat, cede, and claim behavior while keeping
request volume within the rate budget.
- Around line 417-422: In the claim-handling flow around issues::add_labels,
make writing the claimed label best-effort by handling its error locally instead
of propagating it with ?. Preserve the committed claim result and continue
processing remaining issues, matching the removal path’s bookkeeping behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44c228f1-e8e8-436d-b987-2544ee1b7313
📒 Files selected for processing (16)
.superpowers/sdd/progress.mdsrc/cli/serve.rssrc/github/bridge/claim.rssrc/github/bridge/config.rssrc/github/bridge/items.rssrc/github/bridge/marker.rssrc/github/bridge/mod.rssrc/github/bridge/runner.rssrc/github/bridge/tests/live_github.rssrc/github/bridge/tests/two_instance.rssrc/github/bridge/tick.rssrc/github/client.rssrc/github/issues.rssrc/github/mod.rssrc/github/models.rssrc/github/test_support.rs
C2 persisted only the discriminator; the other half came from
claims::agent_of(claims::owner_id()), i.e. whichever agent happens to be
detected at startup. Started from a Claude Code session that is `claude-code`;
started by launchd or systemd nothing is detected and it falls back to `cli`.
Same machine, same home directory, same persisted file -- two different owner
ids. And since marker liveness is judged per owner id, the second run cedes
everything the first was still holding.
That is the C2 mass-cede intact, just triggered by a change of launcher
instead of a change of pid, and it is the more likely trigger of the two: the
daemon is normally started by a service manager, and a developer restarting
it by hand from an agent session flips the identity both ways.
Reproduced against a real repo before the fix:
run A (agent auto-detected): claimed [3, 2] owner claude-code:d8b8bdfc308f
run B (AGENTFLARE_AGENT=cli): ceded [3, 2] owner cli:d8b8bdfc308f
...and after it, run B cedes nothing and posts no cede marker.
The prefix is now the constant `bridge`, and the whole id is read verbatim
from the file, so nothing about it is derived from the environment. That is
also more accurate: the actor is the bridge daemon, not whichever agent
started it. A file written in the old format (no colon) is adopted with the
prefix added rather than re-minted, so an existing workstation keeps one id.
The test asserts the property structurally -- the returned id must equal the
file's contents byte for byte, and the detected agent name must not appear in
it -- rather than mutating process-global env, which is unsafe and would race
every other test in the binary.
…h, id hygiene Six findings from the PR review, verified against the code before fixing. A `done` used to end resolution permanently. A human can reopen a completed issue, which puts it back in the `state=open` queue with its marker history intact -- and `resolve_holder` then answered `None` on every tick forever. An instance with no local item for it posted a claim comment, failed its own `i_hold` re-check because the answer was still `None`, and repeated that every interval. One claim comment per tick, indefinitely, and the work never started. A `done` now ends the issue only while it is the last word. Fixing that exposed a second half: the original owner's claim is still the LOWEST comment id, so it out-ranked every later claimant while its own item sat `completed` and would never be worked again. `Done` now retires a claim exactly as `Cede` does -- both mean "I am no longer working this". Liveness is now bounded on both sides. `now - ts <= ttl` is true for every FUTURE ts, because the difference is negative, so one machine with a fast clock pinned its issues indefinitely and nobody could take them if it died. (The review's suggested `ts.min(now)` clamp does not fix this -- it makes the age 0, i.e. maximally live. Implemented as a real two-sided bound instead.) The `done` comment and the issue close shared one export latch, so a close that failed on its own rolled back a comment that HAD been posted -- and the next tick, seeing the same hash dirty again, posted a second "Completed by ...". Every failed close added another duplicate, and close is the call most likely to fail alone. The close is now tracked separately, with a still-open issue as its own retry signal. store_hash merged into the tick's stale metadata snapshot and wrote the whole column, so an item(update) through the MCP server in that window was silently reverted. It now re-reads immediately before writing, and the rollback path clears just its own key instead of restoring the whole snapshot. An instance id from AGENTFLARE_BRIDGE_INSTANCE_ID was taken verbatim. Whitespace in it splits a marker field in two, so `Marker::parse` fails closed and every marker that instance writes becomes invisible -- and `x ts=99999999999` forges its own liveness outright. A `/` silently retargets the label-delete path, which then 404s and is treated as success while the stale label stays. Rejected at the source, with a test that anything we MINT passes that same validation and round-trips through the marker format. Also: log the missing `started` state instead of returning silently (the claim comment is already posted by then, so a silent bail strands the issue for a TTL with no explanation), and drop a machine-local path containing a username from the tracked progress notes. The owner prefix is now `flared` rather than `bridge`. Nothing parses it -- the colon is not load-bearing -- so it earns its place by being readable in a `claimed:flared:9f2c...` label, and naming the daemon is more accurate than naming the subsystem inside it. Re-verified live against the scratch repo: full lifecycle and the two-instance cede both still pass.
This was referenced Aug 9, 2026
Merged
3 tasks
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.
Issues labelled
agentflarebecome an open pull queue that any agentflare instance may claim, using GitHub itself as the coordination substrate. Two instances on different workstations share no database, so GitHub's comment list is the one medium that can arbitrate between them — and it is the only question GitHub is treated as authoritative for.Off by default.
AGENTFLARE_BRIDGE_ENABLED=1opts in; with it unset nothing in this diff runs.How it works
Every comment the bridge authors carries a hidden HTML-comment footer:
Both instances authenticate to GitHub as the same user, so the GitHub actor cannot tell them apart — or tell an agent from a human. The marker carries that discriminator in-band. Claims resolve by lowest comment id (monotonic;
created_atis second-granular and subject to cross-machine clock skew), and an owner is live while its newest marker is inside the claim TTL.Parsing fails closed: anything unparseable is treated as human-authored, so a malformed body can never halt the poll loop.
One tick is: re-verify and cede what we lost → export what we still hold → claim new work up to remaining headroom. That order matters — a lost race is dropped as early as possible, and headroom reflects everything already taken this tick.
Why the diff is this size
The feature was built over 8 tasks, then a whole-branch review found 2 Critical + 5 Important + 6 Minor defects and it was held back from shipping. The last 9 commits fix all of them, plus 3 more found afterwards. The fixes sit on top of a feature that was never merged, so this cannot be split.
The root problem was that the feature had no heartbeat.
claim.rsdocumented that a progress marker keeps a claim alive, andtick.rsceded anything past the TTL — but the only thing emitting markers wasexport_if_dirty, which fires on a content-hash change. An instance working an issue for 30 minutes watched its own marker go stale, ceded its own live work to nobody, cancelled the local item, and could never reclaim it. A pure wall-clock timer: one instance, no adversary, no network fault.Notable fixes:
resolve_holderorders by is unchanged and a held issue does not collect a bookkeeping comment every 15 minutes.~/.agentflare/bridge-instance-id.cedebuilt its marker withhash: "", which renders ashash=, andparsefails closed on empty values. No other instance could ever see a claim being given up. Caught by the rewritten two-instance test; all 14 unit tests missed it because their fixtures used a non-empty hash the production code never wrote.let _ =. They now write locally before the GitHub call and gate it, with rollback if the remote write fails — so nothing can be posted that we then fail to record and repost every 60s forever.current_dir, repo resolution and project resolution all vanished through bare.ok()?. Repo resolution derives from cwd, and neither the launchd plist nor the systemd unit sets a working directory, so underagentflare daemon startthe bridge was enabled and then silently never ran.AGENTFLARE_BRIDGE_REPO=owner/reponow overrides it, and every path says why it stopped.Testing
72 bridge tests.
cargo test --workspace= 1026 passed, 0 failed.two_instance.rsdrives two real instances — two backends, two ledgers, two owner ids — against one mock GitHub whose comment list, labels and open/closed state are genuinely mutable shared state, all through the realrun_once. It replaces five tests that calledi_holdon hand-built vectors and proved nothing (a ^ bis a property ofOption's return type, not of the protocol; all five passed while every Critical was live).Also verified against a real private scratch repo, because one class of question lives in GitHub's behavior rather than ours.
live_github.rsis#[ignore]d and gated onAGENTFLARE_LIVE_GITHUB_REPO:It asserts against
body_html— what GitHub renders — and confirmed the marker is invisible: a claim comment renders as only<p>Claiming this for <code>owner</code>.</p>, with the footer absent. It also confirmed the heartbeat edits a comment in place without changing its id, that aclaimed:label survives path encoding on the delete endpoint, and that a daemon restart does not cede its in-flight work — with a negative control (changed identity →ceded [3, 2]) proving that last result isn't vacuous.That live run found three further defects, all fixed here: the bridge reacting to its own
claimed:label as a content change, exporting a just-imported item, and a create-race in the persisted id file that would have silently reintroduced the mass-cede.Risk
Off by default, so the blast radius with the flag unset is zero. With it on, the bridge writes comments and labels on issues carrying the queue label.
AGENTFLARE_BRIDGE_MAX_CLAIMS=0is a documented drain mode: stop taking new work, keep re-verifying and exporting what is already held.No new dependencies.
Summary by CodeRabbit
New Features
Bug Fixes
Tests