docs(adr): add polling-based work discovery ADR - #2409
Conversation
Site previewPreview: https://ba134e3d-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 10:07 AM UTC · Completed 10:21 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Previous run (2)Looks good to me Labels: PR adds a new ADR and updates architecture/roadmap docs for the dispatch subsystem Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsHigh
Medium
Low
Info
|
d18e9a7 to
cb73aef
Compare
|
Rebased onto latest
Please re-review when CI is green. |
|
🤖 Finished Review · ✅ Success · Started 11:35 AM UTC · Completed 11:47 AM UTC |
|
I don't like the proposed conditionals. I would go with CEL expressions and have each poll_provider pass some native information. The information passed would be defined by the platform itself, and we would just forward it. This decouples our code a bit from the platforms, their version and breaking changes. A good starting point would be author details under |
That means that event processing and routing needs to happen in the Go code - not necessarily how I see it. I want to keep the option to convert the logic into something that is native to the execution platform or event source at runtime. |
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — 10-agent review
Agents dispatched: 10 (3× Claude-coder, 3× Claude-researcher, 2× Gemini, 2× Codex)
Unique findings posted inline: 17 (CRITICAL: 1, HIGH: 4, MEDIUM: 12)
Updated in-place: 1 (stale-lock-threshold, updated from earlier review)
Already posted (skipped): 4 (race-condition, parity-mapping-inaccuracy, internal-inconsistency, authorization-claim-inconsistency)
Findings are posted as inline comments below. See each comment for severity, details, and suggested fixes.
Assisted-by: Claude (review), Gemini (review), Codex (review)
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — #2409
Agents dispatched: 10 (4× claude-coder, 4× claude-researcher, 1× gemini-code-review, 1× cursor-code-review)
Models used: Claude, Gemini, Codex
Total findings posted: 17 (1 critical, 4 high, 12 medium) — after dedup and false-positive removal
False positives removed: 7
See inline comments for details. Key themes:
- ADR number collision (critical) — 0049 already taken on main; must renumber to 0055
- Jira API constraints underestimated — no server-side changelog filter, points-based rate limits, elevated permissions for group/role lookups
- Lock mechanism gaps — write-then-verify is not optimistic locking, namespace collisions across repos, no cancel mechanism
- Authorization parity — ADR 0054 (already accepted) requires authorization on all dispatch paths; this ADR must comply
- Premature architecture.md strikethrough — open question only partially decided
Assisted-by: Claude (triage, verification), Gemini (review), Codex (review)
I understand... I'm worried about the complexity we will need to assume in order to support all the conditions the platform supports. But we can try it out. |
ADR rewrite summary (
|
| Before | After |
|---|---|
| ~790-line ADR with poll-side triggers, actor blocks, parity table | ~500 lines focused on discovery + coordination |
poll_drivers[].agent_mappings[].triggers in config |
Dropped — routing is harness CEL per ADR 0061 |
| Separate invocation driver / stage routing | fullsend poll uses the same driver architecture as fullsend dispatch |
fullsend watch (deferred separate command) |
fullsend poll --watch (flag reserved; impl deferred) |
| “Optimistic locking” | Write-then-verify (Jira has no CAS); idempotency is mandatory |
| Stale lock 300s | 900s default + two-phase lock guidance |
lastCheck before dispatch |
lastCheck only after successful dispatch |
| Global lock keys | Repo-namespaced fullsend.poll.{owner}.{repo}.* |
Pipeline (new mental model)
fullsend poll [--watch]
→ poll input driver (jira-poll): discover + lock + emit NormalizedEvent
→ fullsend dispatch core: authorize (ADR 0054) → harness CEL (ADR 0061)
→ output driver (gha-dispatch): dispatch directly, not JSON plan
Review feedback addressed
- ADR number collision → 0062
- ADR 0054 / authorization → delegated to dispatch core (not poll config)
- JQL entity-property indexing → client-side lock filter documented
- Trigger payload / slash args →
NormalizedEvent.transition.comment - M/N backlog starvation, rate limits, lock handoff TOCTOU → documented
architecture.md/roadmap.mdupdated; no strikethrough overclaim
Latest commit: Jira NormalizedEvent extension
Added jira-poll-adapter.md — field mapping, transition table, actor rules, example fixture, CEL sample. repo stays the target GitHub repo; Jira issue is entity with key + numeric id.
Schema follow-up (non-blocking): add source.system: jira and optional entity.key to normalized-event.schema.json.
Open questions — triage
Deferred items are now a table in ADR 0062 with resolution owner:
- Implementation epic #2263 — config schema,
gha-dispatch, credentials, lock refresh,--watchdefaults - Small schema PR — Jira enum +
entity.keyin normative JSON schema - Post-MVP issues —
poll cancel, GitHub/GitLab poll drivers, dev-panel linked PRs, formal metrics (#896)
Please re-review when CI is green.
|
🤖 Review · |
ef757e7 to
7c3f126
Compare
|
🤖 Review · |
|
🤖 Finished Review · ❌ Failure · Started 10:48 AM UTC · Completed 11:14 AM UTC |
Per-repo polling via fullsend poll input drivers (Jira first) and the shared fullsend dispatch pipeline. Harness CEL triggers (ADR 0061) own routing; poll drivers emit NormalizedEvents and write-then-verify locks. Update architecture.md and roadmap.md cross-references. Fixes fullsend-ai#2263 Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Define jira-poll adapter mapping in normative docs, link from ADR 0062, and triage remaining open questions to implementation follow-ups. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add source.system jira, optional entity.key (required for Jira events), jira-fs-triage-comment example fixture, and align adapter docs with schema. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add fullsend poll cancel to ADR 0062, defer exportable metrics to post-MVP, drop dev-panel scope from Jira adapter, and rely on GHA concurrency groups for duplicate dispatch mitigation instead of a new idempotency mandate. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Per review: poll --watch is the only named interface; no need to reference a separate watch subcommand that was never released. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
37cdced to
da94ec9
Compare
|
🤖 Review · |
0062 is now dispatch-version-skew on main; update cross-references in architecture, roadmap, and normative Jira poll docs. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ❌ Failure · Started 6:47 AM UTC · Completed 7:01 AM UTC |
ggallen
left a comment
There was a problem hiding this comment.
Review
Findings
Medium
-
[design] Two-phase lock deferred but load-bearing —
0063-polling-based-work-discovery.md~L394: "Consider a two-phase lock" is phrased as a suggestion in the Decision section. The handoff gap between poller exit and runner first refresh is the primary duplicate-dispatch failure mode. If GHA queue latency exceedsstale_threshold − runner_startup − refresh_interval/2, a second poller steals the lock. The poll-trigger workflow "SHOULD refresh the lock as its first step" (~L299) uses SHOULD rather than MUST. This should either be decided (pending→running phases) or explicitly moved to Open Questions with the risk documented. -
[design] Agent idempotency stated as SHOULD but is the actual safety net —
0063~L355: "Agent implementations SHOULD still be safe to re-run." Since GHA concurrency groups have known gaps (cross-stage races, pre-group-entry window), idempotency is not optional defense-in-depth — it's the real safety net for duplicate dispatch. Consider MUST, or at minimum document why SHOULD is acceptable. -
[design] Config drift: orphaned JQL queries — Poll discovery config lives in
config.yamlwhile routing lives in harness CEL triggers. If a harness is removed but its JQL query remains, the poller discovers work, generates NormalizedEvents, pays the API cost, but matches zero harnesses. No validation or warning is specified to detect this drift. -
[completeness] Missing consequence: credential surface area — The ADR introduces Jira API token dependencies into per-repo config (a new credential to manage and rotate), but Consequences doesn't list this. It appears only in Open Questions. Deserves an explicit negative consequence.
Low
-
[cross-ref] ADR number collision risk — Three open PRs (#2409, #2583, #2743) all claim ADR number 0063. Whichever merges first owns it; the others must renumber. Recommend running
/renumber-adrbefore merge. -
[cross-ref] Minor title truncations in References — ADR 0016 listed as "Unidirectional control flow" (actual: "…through the execution stack"). ADR 0041 omits "agent" from "…event-driven agent dispatch." Non-blocking.
-
[schema]
entity.keypattern is Jira-specific — Pattern^[A-Za-z][A-Za-z0-9_]+-\d+$doesn't allow hyphens in the prefix portion. Fine for Jira, but the field description says "Human-readable work item key when the source system uses one" — may need loosening when future systems are added. -
[style] "Consider a..." in Decision section — L394's advisory language in a section that should be declarative. Either commit to it or move to Open Questions.
-
[completeness]
fullsend poll cancellacks--dry-run— As an operational recovery tool used under pressure, previewing what would be cancelled before acting is standard practice. -
[completeness] API budget section is directional — Provides no per-cycle budget estimate. With M=50 candidates and client-side lock filtering, operators have no baseline for tuning.
Schema & Consistency
- Schema changes are valid JSON Schema 2020-12. The
if/thenconditional forentity.keywhensource.systemisjirais correctly structured. - Example fixture validates against the updated schema.
- Inline example in adapter doc matches the fixture file.
- Transition kinds in adapter doc are all valid per the schema enum.
- Environment variables are consistent between ADR and adapter doc.
- All cross-reference links resolve correctly across all 7 files.
Verdict
Approve. The architecture is sound — the composition model over ADR 0061 is clean, the dual-layer duplicate mitigation is pragmatic, and per-repo scoping is appropriate. The medium findings are real design gaps but are mitigatable during implementation. None block merge for a docs-only ADR.
|
🤖 Finished Retro · ✅ Success · Started 10:30 PM UTC · Completed 10:42 PM UTC |
|
PR #2409 added ADR 0063 (polling-based work discovery) across a 17-day lifecycle. The review agent ran 7+ times (2 successful, 3 cancelled, 2 failed-but-analysis-completed), approving after a major rewrite. Two human reviewers also approved but caught substantive design gaps the agent missed: RFC 2119 keyword misuse on load-bearing mechanisms, incomplete operational consequences, config drift risks, and advisory language in the Decision section. The agent's findings were limited to surface-level inconsistencies. One proposal targets enriching AGENTS.md ADR review guidance with the specific analytical checks humans applied and the agent missed. Related existing issues: #1480 (shallow docs feedback), #1469 (missing feature-level design assessment). Proposals filed
|
Fixes #2263
Summary
fullsend pollinput drivers (Jira first) on the same driver architecture asfullsend dispatch(ADR 0061).NormalizedEventvalues; harness CELtriggerexpressions own routing (no poll-side trigger config).gha-dispatchoutput driver.lastCheckonly on successful dispatch,fullsend poll --watchandfullsend poll cancel.NormalizedEventschema extension + jira-poll-adapter.docs/architecture.mdanddocs/roadmap.md.Test plan