feat: Buttons Flow runtime with research-deck example - #285
Open
bobakemamian wants to merge 3 commits into
Open
Conversation
Compile drawer_kind:flow in memory at press time into the existing executor pipeline, ship local/GitHub provider buttons and flow CLI sugar, and replace the SWE demo with @buttonsflow/research-deck for open-slide workflows. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe pull request adds flow board management for local and GitHub providers. It adds flow drawer compilation, task lifecycle scripts, approval gates, scheduling, built-in research-deck support, and integration coverage for execution and recovery. ChangesFlow execution and management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FlowCLI
participant Flowkit
participant Provider
User->>FlowCLI: initialize or press flow board
FlowCLI->>Flowkit: ensure provider buttons and task store
FlowCLI->>Provider: list and claim actionable task
Provider-->>FlowCLI: task claim result
FlowCLI->>Provider: perform, validate, and apply stage result
Provider-->>User: status, approval, or completion result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 18
🧹 Nitpick comments (13)
internal/store/builtin.go (3)
148-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFall back to the primary source for unknown
@buttonsflow/packages.Line 149 routes every
@buttonsflow/name toBuiltinSource.BuiltinSource.Fetchonly serves@buttonsflow/research-deck. A registry-hosted package such as@buttonsflow/othertherefore fails, even whenPrimarycan serve it.♻️ Proposed fallback
func (s PreferBuiltin) Fetch(name, version string) (*Bundle, error) { if strings.HasPrefix(name, "`@buttonsflow/`") { - return (&BuiltinSource{}).Fetch(name, version) + b, err := (&BuiltinSource{}).Fetch(name, version) + if err == nil || s.Primary == nil { + return b, err + } } if s.Primary == nil { return nil, fmt.Errorf("package %q not found", name) } return s.Primary.Fetch(name, version) }🤖 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 `@internal/store/builtin.go` around lines 148 - 156, Update PreferBuiltin.Fetch so `@buttonsflow/` names first use BuiltinSource, then fall back to s.Primary.Fetch when the builtin lookup reports the package is unavailable. Preserve the existing not-found error when Primary is nil, and continue routing non-@buttonsflow/ names directly through Primary.
136-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface or document the discarded primary index error.
Line 141 receives an error from
s.Primary.Index(), and line 143 returnsnil. A registry outage then looks like an empty catalog.golangci-lint(nilerr) flags this. If the fallback is intentional, add a comment that states the intent, and log the error. If it is not intentional, propagate the error.♻️ Proposed change to keep the fallback and record the cause
primary, err := s.Primary.Index() if err != nil { + // Degrade to builtin-only when the registry is unreachable, but do + // not hide the cause from the operator. + fmt.Fprintf(os.Stderr, "warning: registry index unavailable: %v\n", err) return builtin, nil }
fmtis already imported; addosto the import block.🤖 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 `@internal/store/builtin.go` around lines 136 - 146, Update PreferBuiltin.Index to handle the error returned by s.Primary.Index instead of silently returning the builtin catalog: preserve the fallback only if intentional, document it at the error branch, and log the discarded error using the suggested os-based mechanism; otherwise propagate the error. Ensure the nilerr warning is resolved.Source: Linters/SAST tools
60-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe research-deck board is defined twice, and the two definitions have already drifted.
researchDeckDrawer()builds the board as adrawer.Drawerliteral.installResearchDeckBoard()rebuilds the same stage graph, worker agents, timeouts, gate, manager, and initial stage throughAddFlowStageandSetFlowField. The stage prompts already differ: the builtin definition states the advance condition for each stage, and the CLI definition omits it. A user who installs the package therefore gets a different board than a user who runsbuttons flow init research-deck.Export one canonical definition and derive both paths from it.
internal/store/builtin.go#L60-L128: move this literal into a shared exported constructor, for exampledrawer.ResearchDeckDefinition(), and call it here.cmd/flow_research_deck.go#L22-L63: replace the stage loop and thesetsmap with a single persist of the shared definition, so the CLI path and the package path produce identical boards.🤖 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 `@internal/store/builtin.go` around lines 60 - 128, The research-deck flow is defined independently in two locations and has drifted. In internal/store/builtin.go:60-128, move the literal from researchDeckDrawer into a shared exported constructor such as drawer.ResearchDeckDefinition, and have researchDeckDrawer return that definition. In cmd/flow_research_deck.go:22-63, remove the duplicated stage loop and sets map, then persist the shared definition directly so both paths produce identical boards.cmd/flow_research_deck.go (1)
37-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the flow fields in a deterministic order.
setsis a map, so line 64 iterates in randomized order. Two consequences follow. First, a partial failure leaves a different on-disk state on each run, which makes install failures hard to reproduce. Second,SetFlowFieldpersists the drawer on each call, so this loop performs 26 sequential writes for one install.Use an ordered slice of path/value pairs. A single batched update would also remove the repeated writes, if the service exposes one.
♻️ Proposed change to an ordered slice
- sets := map[string]any{ - "initial_stage": "brief", - "manager.agent": "activation.manager", + sets := []struct { + path string + value any + }{ + {"initial_stage", "brief"}, + {"manager.agent", "activation.manager"}, ... } - for path, value := range sets { - if _, err := svc.SetFlowField("research-deck", path, value); err != nil { - return fmt.Errorf("set flow.%s: %w", path, err) + for _, s := range sets { + if _, err := svc.SetFlowField("research-deck", s.path, s.value); err != nil { + return fmt.Errorf("set flow.%s: %w", s.path, err) } }🤖 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 `@cmd/flow_research_deck.go` around lines 37 - 68, Replace the unordered sets map and range loop in the flow setup with an ordered slice of path/value pairs, preserving the current field order and values so SetFlowField applies changes deterministically. If the service exposes a batch flow-field update API, use it to persist the complete set in one operation; otherwise retain sequential SetFlowField calls over the ordered slice and existing error wrapping.internal/flowkit/install.go (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
claimWaitSecondsis unused.golangci-lint reports the constant as dead. The scripts hardcode the same default through
BUTTONS_FLOW_CLAIM_WAITwith fallback"1". Either remove the constant or inject it into the generated script bodies so one value defines the wait.🤖 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 `@internal/flowkit/install.go` at line 18, Resolve the unused claimWaitSeconds constant by either removing it or using it when generating the script bodies’ BUTTONS_FLOW_CLAIM_WAIT fallback. Ensure the wait default is defined in one place and remains 1.Source: Linters/SAST tools
internal/drawer/entity.go (1)
160-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the field
HeartbeatSecondsfor consistency.
FlowManagerusesHeartbeatSecondsfor the sameheartbeat_secondsJSON key.FlowRole.Heartbeatdiverges from that convention. Rename it now, while no external code depends on the Go field.🤖 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 `@internal/drawer/entity.go` at line 160, Rename the FlowRole field from Heartbeat to HeartbeatSeconds while preserving the `heartbeat_seconds` JSON tag, and update any references to the field accordingly. Keep the existing type and omitempty behavior unchanged.internal/drawer/schema_embedded.json (1)
154-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
rolesis generated as an untyped object in both schema artifacts.FlowDefinition.Rolesismap[string]FlowRoleininternal/drawer/entity.go, but the generator emits a bare"type": "object", so no role field is validated. Both files are produced from that one struct annotation.
internal/drawer/schema_embedded.json#L154-L157: after adding aFlowRole$defand wiringadditionalPropertiesto it in the Go annotation, regenerate this embedded copy withgo generate ./....docs/schemas/drawer.schema.json#L154-L157: regenerate the canonical schema from the same run so the two artifacts stay identical.As per coding guidelines: "The canonical JSON Schema lives at
docs/schemas/drawer.schema.jsonand is generated from the Go struct viago generate ./...".🤖 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 `@internal/drawer/schema_embedded.json` around lines 154 - 157, Update the Go schema annotation for FlowDefinition.Roles in internal/drawer/entity.go to define FlowRole and set roles.additionalProperties to the FlowRole schema, so each role field is validated. Regenerate both internal/drawer/schema_embedded.json:154-157 and docs/schemas/drawer.schema.json:154-157 with go generate ./...; both sites require generated updates and must remain identical.Source: Coding guidelines
cmd/serve.go (1)
427-432: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCompile before the 202 response so senders learn about failures.
PrepareForExecuteruns inside the goroutine. The handler always answers202 Acceptedon Line 446, so a flow drawer with an invalid definition produces a success response and one stderr line. Compilation is deterministic and does not perform I/O, so it can run before the goroutine starts. A prep failure can then return500with the error code.♻️ Proposed direction
+ execDrawer, prepErr := drawer.PrepareForExecute(d) + if prepErr != nil { + http.Error(w, `{"ok":false,"error":"drawer_compile_failed"}`, http.StatusInternalServerError) + return + } + h.wg.Add(1) go func() { defer h.wg.Done() ctx, cancel := context.WithTimeout(h.pressCtx, time.Hour) defer cancel() exec := drawer.NewExecutor() - execDrawer, prepErr := drawer.PrepareForExecute(d) - if prepErr != nil { - fmt.Fprintf(os.Stderr, "[serve] drawer %s compile error: %v\n", d.Name, prepErr) - return - } result, execErr := exec.Execute(ctx, execDrawer, map[string]any{"webhook": webhookInput})🤖 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 `@cmd/serve.go` around lines 427 - 432, Move the PrepareForExecute call for the webhook flow out of the goroutine and execute it before the handler sends the 202 response. In the surrounding serve handler, return HTTP 500 with the appropriate error code when preparation fails; only launch the goroutine and call exec.Execute after successful preparation, while preserving the existing execution-error handling.internal/drawer/compile_flow_test.go (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Asfor the error type check.
err.(*ServiceError)fails ifCompileFlowlater wraps the error with%w.errors.Askeeps the test correct across that change.♻️ Proposed change
- se, ok := err.(*ServiceError) - if !ok || se.Code != "VALIDATION_ERROR" { + var se *ServiceError + if !errors.As(err, &se) || se.Code != "VALIDATION_ERROR" { t.Fatalf("err = %#v", err) }Add
"errors"to the import block.🤖 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 `@internal/drawer/compile_flow_test.go` around lines 70 - 73, Update the error assertion in the CompileFlow test to use errors.As with a *ServiceError target, preserving the existing validation-code check and failure message; add the errors import required for this assertion.internal/flowkit/claim_test.go (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the tasks path from the shared helper.
The test hard-codes
home/flows/<board>/tasks.config.FlowBoardDiralready owns that layout (see cmd/flow.go Line 314). If the layout changes, this test breaks silently rather than following the helper.🤖 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 `@internal/flowkit/claim_test.go` at line 28, Update the task path construction in the relevant test to use the shared config.FlowBoardDir helper instead of manually joining home, “flows”, board, and “tasks”. Preserve the existing tid-based JSON filename while delegating the directory layout to config.FlowBoardDir.test/integration/flow_drawer_test.go (1)
185-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the press assertion.
ExecuteResultalways serializes astatusfield. The nested condition therefore passes for a failed run as well as a successful run, so the check adds nothing beyond the exit-code check at Line 182. The test name states that the flow compiles and runs, so assert the successful status directly.🧪 Proposed change
- if !strings.Contains(r.Stdout, `"ok"`) && !strings.Contains(r.Stdout, `"status": "ok"`) { - // ExecuteResult uses status field - if !strings.Contains(r.Stdout, `"status"`) { - t.Fatalf("unexpected press output: %s", r.Stdout) - } - } + if !strings.Contains(r.Stdout, `"status": "ok"`) { + t.Fatalf("expected a successful press result: %s", r.Stdout) + }🤖 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 `@test/integration/flow_drawer_test.go` around lines 185 - 190, Update the press-output assertion in the flow compilation/run test to require a successful status value directly, rather than merely checking for the presence of the "status" field. Remove the redundant nested condition and preserve the existing failure message for outputs that do not indicate success.internal/drawer/service.go (1)
194-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing the provider allow-list.
SetFlowFieldhard-codes"local"and"github".flowkit.EnsureButtons(internal/flowkit/install.go:22-35) encodes the same set. A third provider requires edits in both places. Export a singleflowkit.ValidProvider(name)helper and call it here.🤖 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 `@internal/drawer/service.go` around lines 194 - 204, Centralize provider validation by adding the exported flowkit.ValidProvider(name) helper with the local and github allow-list, then update SetFlowField’s "provider" case to use it instead of hard-coding those values. Preserve the existing type validation, error behavior, and assignment for valid providers, and reuse the helper from flowkit.EnsureButtons so the allow-list has one source of truth.test/integration/flow_recovery_test.go (1)
89-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the stale claim was actually replaced.
The test verifies only that the status reached
done. A provider that ignores claims entirely also passes. The test name states recovery through staleness, so also assert thatflow.claimed_byno longer holdscrashed-agent.🧪 Proposed addition
if status != "done" { t.Fatalf("expected status=done after reclaim press, got %#v task=%s", status, data) } + if props != nil { + if holder, _ := props["flow.claimed_by"].(string); holder == "crashed-agent" { + t.Fatalf("stale claim was not released: task=%s", data) + } + } }🤖 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 `@test/integration/flow_recovery_test.go` around lines 89 - 99, Extend the assertions in the recovery test around the existing status validation to read the task’s flow claim and verify that claimed_by is no longer "crashed-agent". Keep the current status=done assertion, and fail the test with relevant task data if the stale claim remains.
🤖 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 `@cmd/flow_research_deck.go`:
- Around line 65-67: Update flowInit’s handling of installResearchDeckBoard
failures to return handleDrawerError(err) instead of the raw error, ensuring
template-install failures use the required JSON envelope with --json.
In `@cmd/flow.go`:
- Around line 106-117: Declare a separate flowReason string variable and update
flowRejectCmd to read and pass flowReason as the reject reason instead of
flowFilter. Keep flowFilter reserved for flowTaskCmd’s --filter flag and
preserve the existing omission behavior when no reason is provided.
- Around line 479-504: Replace the direct drawer.json read/modify/write hack in
the trigger-clearing flow with an exported drawer service method such as
ClearTriggers(name). Implement ClearTriggers using the service’s normal load,
validation, mutation, updated_at refresh, and save path, then call it from this
flow and remove the dead d.Triggers/d.UpdatedAt assignments and filesystem
manipulation.
- Around line 321-350: Update the flow generation logic around the schedule
metadata and GitHub workflow so the declared poll interval matches the
provider’s actual schedule. Introduce a shared provider-based interval value—300
seconds for GitHub and 60 seconds otherwise—and derive both schedule.json’s
every_seconds field and the GitHub cron expression from it, preserving the
existing workflow behavior.
- Around line 466-469: Replace the raw JSON printing loop over filtered runs
with a human-readable formatted summary using the field names defined by
drawer.Run. Keep machine-readable output exclusively in the existing jsonOutput
branch via config.WriteJSON. Update the board-name validation error to use the
coded error mechanism with the uppercase code MISSING_ARG.
In `@internal/drawer/compile_flow.go`:
- Around line 81-90: The compiled drawer in the compilation flow must use the v2
schema version to match its DrawerKindAction value. Update the SchemaVersion
field in the Drawer construction to use the SchemaVersion constant or
d.SchemaVersion, while leaving the remaining fields unchanged.
- Around line 66-74: Update the parallelism calculation in the compile flow to
enforce Limits.MaxActiveTasks as an upper bound: when processing each stage,
clamp its Concurrency to the configured max before determining the final
parallelism. Preserve the default and unlimited behavior when no positive
max_active_tasks is configured.
In `@internal/flowkit/install.go`:
- Around line 97-122: The installOne replacement flow must preserve the existing
button when svc.Create fails. Update installOne to retain the current definition
and restore it after a failed replacement, or create under a temporary name and
swap only after success; also handle and propagate the svc.Remove error instead
of discarding it.
- Line 71: Update the flow-github-task-claim registration in install.go to
include the same QueueConfig used by flow-github-claim and the corresponding
local task pair, keyed on task_id. Keep githubClaimCode and the existing
arguments unchanged so manual claims use identical serialized queue behavior.
In `@internal/flowkit/scripts_github.go`:
- Around line 170-179: Update githubTaskListCode, githubTaskReadCode,
githubTaskUpdateCode, githubTaskRmCode, githubTaskCommentCode,
githubApproveCode, and githubRejectCode to validate that repo is set before
invoking subprocess.run. Reuse the same guard and structured JSON error response
already used by githubTaskAddCode, returning it immediately when both repository
environment variables are absent.
- Around line 254-275: Update githubApproveCode, githubRejectCode, and
githubApplyCode to capture each gh subprocess result, inspect returncode, and
include captured stderr in a failure response with ok: false. Only emit the
existing ok: true response after every required GitHub operation succeeds,
preserving the scripts’ current success payloads.
- Line 139: The claim-release logic around githubClaimCode must remove the same
login that was assigned, rather than always using gh’s authenticated `@me` alias.
Pass the resolved holder through to the subprocess call or recompute it using
the exact BUTTONS_FLOW_HOLDER, GITHUB_ACTOR, then buttons-agent precedence, and
use that value for --remove-assignee.
- Around line 79-99: Update the claim checks in the issue-view flow to compare
the full assignee login set rather than only assignees[0]. In both the initial
existing-claim check and the post-edit verification, ensure the claim succeeds
only when holder is the sole assignee; treat any additional assignee as already
claimed or lost_race respectively.
In `@internal/flowkit/scripts_local.go`:
- Line 90: Replace every direct task JSON write in the identified script
locations with an atomic same-directory temporary-file workflow: write the
complete content, set the temporary file mode to 0o600, then use os.replace to
overwrite the target. Apply this consistently to all task-writing paths,
including the writes near lines 90, 193, 204, 216, 270, 347, 381, 402, and 424,
while preserving the existing JSON formatting and trailing newline.
- Line 335: Add the same task-file existence check used by localTaskReadCode to
localTaskUpdateCode, localTaskCommentCode, localApproveCode, and localRejectCode
before reading or parsing the file; return the structured {"ok": false, "error":
"not_found"} response for missing task IDs instead of allowing FileNotFoundError
tracebacks.
- Around line 200-206: Update the gated advance logic in the branch handling v
== "advance" to require flow.approved_stage to equal from_stage instead of
checking the permanent approved flag, and consume that stage-specific approval
after it is used. Update localApproveCode to store the approved stage in
flow.approved_stage, preserving pending-approval behavior for mismatched or
absent approvals.
- Line 215: Update the comment timestamp expression in the affected script to
use the existing datetime and timezone imports instead of
__import__("datetime").datetime.utcnow(), avoiding duplicate imports. Also
change the file mode from 0644 to 0700.
In `@test/integration/flow_runtime_test.go`:
- Around line 111-124: Replace loose stdout substring matching with JSON-based
assertions in test/integration/flow_runtime_test.go#L111-L124 and
test/integration/flow_drawer_test.go#L185-L190. In the flow runtime
result-counting logic, fail on unparseable payloads and count only items whose
status field equals the requested status; in the flow drawer test, decode the
response and directly assert that the status field is "ok".
---
Nitpick comments:
In `@cmd/flow_research_deck.go`:
- Around line 37-68: Replace the unordered sets map and range loop in the flow
setup with an ordered slice of path/value pairs, preserving the current field
order and values so SetFlowField applies changes deterministically. If the
service exposes a batch flow-field update API, use it to persist the complete
set in one operation; otherwise retain sequential SetFlowField calls over the
ordered slice and existing error wrapping.
In `@cmd/serve.go`:
- Around line 427-432: Move the PrepareForExecute call for the webhook flow out
of the goroutine and execute it before the handler sends the 202 response. In
the surrounding serve handler, return HTTP 500 with the appropriate error code
when preparation fails; only launch the goroutine and call exec.Execute after
successful preparation, while preserving the existing execution-error handling.
In `@internal/drawer/compile_flow_test.go`:
- Around line 70-73: Update the error assertion in the CompileFlow test to use
errors.As with a *ServiceError target, preserving the existing validation-code
check and failure message; add the errors import required for this assertion.
In `@internal/drawer/entity.go`:
- Line 160: Rename the FlowRole field from Heartbeat to HeartbeatSeconds while
preserving the `heartbeat_seconds` JSON tag, and update any references to the
field accordingly. Keep the existing type and omitempty behavior unchanged.
In `@internal/drawer/schema_embedded.json`:
- Around line 154-157: Update the Go schema annotation for FlowDefinition.Roles
in internal/drawer/entity.go to define FlowRole and set
roles.additionalProperties to the FlowRole schema, so each role field is
validated. Regenerate both internal/drawer/schema_embedded.json:154-157 and
docs/schemas/drawer.schema.json:154-157 with go generate ./...; both sites
require generated updates and must remain identical.
In `@internal/drawer/service.go`:
- Around line 194-204: Centralize provider validation by adding the exported
flowkit.ValidProvider(name) helper with the local and github allow-list, then
update SetFlowField’s "provider" case to use it instead of hard-coding those
values. Preserve the existing type validation, error behavior, and assignment
for valid providers, and reuse the helper from flowkit.EnsureButtons so the
allow-list has one source of truth.
In `@internal/flowkit/claim_test.go`:
- Line 28: Update the task path construction in the relevant test to use the
shared config.FlowBoardDir helper instead of manually joining home, “flows”,
board, and “tasks”. Preserve the existing tid-based JSON filename while
delegating the directory layout to config.FlowBoardDir.
In `@internal/flowkit/install.go`:
- Line 18: Resolve the unused claimWaitSeconds constant by either removing it or
using it when generating the script bodies’ BUTTONS_FLOW_CLAIM_WAIT fallback.
Ensure the wait default is defined in one place and remains 1.
In `@internal/store/builtin.go`:
- Around line 148-156: Update PreferBuiltin.Fetch so `@buttonsflow/` names first
use BuiltinSource, then fall back to s.Primary.Fetch when the builtin lookup
reports the package is unavailable. Preserve the existing not-found error when
Primary is nil, and continue routing non-@buttonsflow/ names directly through
Primary.
- Around line 136-146: Update PreferBuiltin.Index to handle the error returned
by s.Primary.Index instead of silently returning the builtin catalog: preserve
the fallback only if intentional, document it at the error branch, and log the
discarded error using the suggested os-based mechanism; otherwise propagate the
error. Ensure the nilerr warning is resolved.
- Around line 60-128: The research-deck flow is defined independently in two
locations and has drifted. In internal/store/builtin.go:60-128, move the literal
from researchDeckDrawer into a shared exported constructor such as
drawer.ResearchDeckDefinition, and have researchDeckDrawer return that
definition. In cmd/flow_research_deck.go:22-63, remove the duplicated stage loop
and sets map, then persist the shared definition directly so both paths produce
identical boards.
In `@test/integration/flow_drawer_test.go`:
- Around line 185-190: Update the press-output assertion in the flow
compilation/run test to require a successful status value directly, rather than
merely checking for the presence of the "status" field. Remove the redundant
nested condition and preserve the existing failure message for outputs that do
not indicate success.
In `@test/integration/flow_recovery_test.go`:
- Around line 89-99: Extend the assertions in the recovery test around the
existing status validation to read the task’s flow claim and verify that
claimed_by is no longer "crashed-agent". Keep the current status=done assertion,
and fail the test with relevant task data if the stale claim remains.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dd01139-7ff5-4e1c-ab82-cc4daa9f12ab
📒 Files selected for processing (22)
cmd/drawer.gocmd/flow.gocmd/flow_research_deck.gocmd/install.gocmd/root.gocmd/serve.godocs/schemas/drawer.schema.jsoninternal/config/paths.gointernal/drawer/compile_flow.gointernal/drawer/compile_flow_test.gointernal/drawer/entity.gointernal/drawer/schema_embedded.jsoninternal/drawer/service.gointernal/flowkit/claim_test.gointernal/flowkit/install.gointernal/flowkit/scripts_github.gointernal/flowkit/scripts_local.gointernal/store/builtin.gotest/integration/flow_drawer_test.gotest/integration/flow_recovery_test.gotest/integration/flow_runtime_test.gotest/integration/helpers_test.go
| props.pop("flow.pending_approval", None) | ||
| if verdict.get("summary"): | ||
| comments = task.setdefault("comments", []) | ||
| comments.append({"body": verdict["summary"], "at": __import__("datetime").datetime.utcnow().strftime("%Y-%m-%dT%H:%M:%SZ")}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is datetime.utcnow deprecated in Python 3.12?
💡 Result:
Yes, datetime.utcnow is deprecated in Python 3.12 [1][2]. The deprecation was introduced because datetime.utcnow returns a naive datetime object (lacking timezone information), which can lead to errors when handled by methods that assume naive objects represent local time [3][4][2]. It is important to note that datetime.now(timezone.utc) is not a direct drop-in replacement, as it returns a timezone-aware object [3][5]. If your existing code requires a naive datetime object for compatibility, you should use datetime.now(timezone.utc).replace(tzinfo=None) instead [3][5].
Citations:
- 1: https://docs.python.org/3/library/datetime.html
- 2: https://docs.python.org/3/_sources/library/datetime.rst.txt
- 3: GH-103857: Deprecate utcnow and utcfromtimestamp python/cpython#103858
- 4: Deprecate
utcnowandutcfromtimestamppython/cpython#103857 - 5:
utcnowdeprecation note is misleading python/cpython#118542
🏁 Script executed:
#!/bin/bash
set -eu
file="internal/flowkit/scripts_local.go"
printf '%s\n' '--- permissions ---'
stat -c '%a %n' "$file"
printf '%s\n' '--- target context ---'
sed -n '1,35p;195,225p' "$file"
printf '%s\n' '--- datetime usage in file ---'
rg -n -C 2 'datetime|timezone|__import__' "$file"
printf '%s\n' '--- related file permissions ---'
find internal -type f -name '*.go' -o -type f -name '*spec*.json' -o -type f -name '*history*.json' | while IFS= read -r p; do
stat -c '%a %n' "$p"
done | head -100Repository: autonoco/buttons
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -eu
file="internal/flowkit/scripts_local.go"
printf '%s\n' '--- permissions ---'
python3 - "$file" <<'PY'
import os, sys
path = sys.argv[1]
print(f"{oct(os.stat(path).st_mode & 0o777)[2:]} {path}")
PY
printf '%s\n' '--- target context ---'
sed -n '1,35p;195,225p' "$file"
printf '%s\n' '--- datetime usage in file ---'
grep -n -C 2 -E 'datetime|timezone|__import__' "$file" || true
printf '%s\n' '--- relevant internal file permissions ---'
find internal -type f \( -name '*.go' -o -name '*spec*.json' -o -name '*history*.json' \) -print | while IFS= read -r p; do
python3 - "$p" <<'PY'
import os, sys
path = sys.argv[1]
print(f"{oct(os.stat(path).st_mode & 0o777)[2:]} {path}")
PY
done | head -100Repository: autonoco/buttons
Length of output: 8863
🏁 Script executed:
#!/bin/bash
set -eu
file="internal/flowkit/scripts_local.go"
python3 - "$file" <<'PY'
import os, re, sys
from datetime import datetime, timezone
path = sys.argv[1]
text = open(path, encoding="utf-8").read()
mode = os.stat(path).st_mode & 0o777
target = next(line.strip() for line in text.splitlines() if '"at": __import__("datetime")' in line)
replacement = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")
print("filesystem_mode:", oct(mode))
print("target_uses_utcnow:", "datetime.utcnow()" in target)
print("target_uses_inline_import:", '__import__("datetime")' in target)
print("replacement_has_utc_suffix:", replacement.endswith("Z"))
print("replacement_contains_offset:", "+00:00" in replacement)
PY
printf '%s\n' '--- tracked mode ---'
git ls-files -s -- "$file"
printf '%s\n' '--- all tracked internal Go modes ---'
git ls-files -s -- 'internal/**/*.go' | awk '{print $1, $4}' | sort | uniq -cRepository: autonoco/buttons
Length of output: 6345
Replace datetime.utcnow() and set the file mode to 0700.
Use the existing datetime and timezone imports. Do not add duplicate imports. internal/flowkit/scripts_local.go currently has mode 0644, but internal Go code files must use mode 0700.
🤖 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 `@internal/flowkit/scripts_local.go` at line 215, Update the comment timestamp
expression in the affected script to use the existing datetime and timezone
imports instead of __import__("datetime").datetime.utcnow(), avoiding duplicate
imports. Also change the file mode from 0644 to 0700.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/drawer/service.go (1)
195-202: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStore the normalized provider value.
Lines 195-202 accept
" github "becauseflowkit.ValidProvidertrims whitespace. The method then stores" github "ind.Flow.Provider. Later provider lookup will not matchgithub.Trim
vbefore validation and assignment.Proposed fix
- } else if !flowkit.ValidProvider(v) { + } else if !flowkit.ValidProvider(strings.TrimSpace(v)) { setErr = fmt.Errorf("must be local or github") } else { - d.Flow.Provider = v + d.Flow.Provider = strings.TrimSpace(v)🤖 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 `@internal/drawer/service.go` around lines 195 - 202, Update the "provider" case to trim the string value into the normalized provider before calling flowkit.ValidProvider, then assign that normalized value to d.Flow.Provider so surrounding whitespace is not stored.internal/flowkit/scripts_github.go (3)
192-204: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn an error when the GitHub list command fails.
Lines 192-193 convert a
gh issue listfailure into[]. The script then reportscount: 0, so callers treat an unavailable repository or authorization failure as an empty board.Exit with an error response when
proc.returncode != 0.🤖 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 `@internal/flowkit/scripts_github.go` around lines 192 - 204, The issue-list flow currently treats a failed gh issue list command as an empty result. Update the subprocess handling before parsing issues so proc.returncode != 0 produces an error response and exits, while preserving the existing JSON parsing and item filtering for successful commands.
242-245: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace the existing status label.
A status patch only adds
status:<new>. It does not remove the priorstatus:<old>label.githubTaskListCodederives status by iterating labels, so a task with multiple status labels has ambiguous state.Read and remove existing
status:labels before adding the requested status label.🤖 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 `@internal/flowkit/scripts_github.go` around lines 242 - 245, Update the status handling in githubTaskListCode so status patches first read the issue’s existing labels and remove every label with the status: prefix, then add the requested status:new label. Preserve the current status extraction and patch response behavior.
239-245: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck GitHub write results before emitting success. Both operations discard failed
ghmutations and return an"ok": trueresponse.
internal/flowkit/scripts_github.go#L239-L245: capture eachgh issue editresult and return an error when any requested update fails.internal/flowkit/scripts_github.go#L257-L258: capture thegh issue closeresult and return an error when closure fails.🤖 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 `@internal/flowkit/scripts_github.go` around lines 239 - 245, Check the results of every requested GitHub mutation before reporting success: in internal/flowkit/scripts_github.go lines 239-245, capture each gh issue edit invocation and return an error if any title, body, or status update fails; in lines 257-258, likewise capture gh issue close and return an error on failure. Only emit the existing ok response after all requested operations succeed.
🤖 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 `@cmd/flow_research_deck.go`:
- Around line 20-29: Make the research-deck installation atomic by updating the
flow around CreateWithKind and Save in the research-deck installation function:
if saving the canonical ResearchDeckDrawer fails, remove the newly created
scaffold before returning the error, or use an existing service operation that
persists the canonical drawer transactionally. Preserve the existing
duplicate-installation check and successful scaffold creation behavior.
---
Outside diff comments:
In `@internal/drawer/service.go`:
- Around line 195-202: Update the "provider" case to trim the string value into
the normalized provider before calling flowkit.ValidProvider, then assign that
normalized value to d.Flow.Provider so surrounding whitespace is not stored.
In `@internal/flowkit/scripts_github.go`:
- Around line 192-204: The issue-list flow currently treats a failed gh issue
list command as an empty result. Update the subprocess handling before parsing
issues so proc.returncode != 0 produces an error response and exits, while
preserving the existing JSON parsing and item filtering for successful commands.
- Around line 242-245: Update the status handling in githubTaskListCode so
status patches first read the issue’s existing labels and remove every label
with the status: prefix, then add the requested status:new label. Preserve the
current status extraction and patch response behavior.
- Around line 239-245: Check the results of every requested GitHub mutation
before reporting success: in internal/flowkit/scripts_github.go lines 239-245,
capture each gh issue edit invocation and return an error if any title, body, or
status update fails; in lines 257-258, likewise capture gh issue close and
return an error on failure. Only emit the existing ok response after all
requested operations succeed.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e55c143-2e6d-4978-b081-73e520490f2d
📒 Files selected for processing (18)
cmd/flow.gocmd/flow_research_deck.gocmd/serve.godocs/schemas/drawer.schema.jsoninternal/drawer/compile_flow.gointernal/drawer/compile_flow_test.gointernal/drawer/entity.gointernal/drawer/schema_embedded.jsoninternal/drawer/service.gointernal/flowkit/claim_test.gointernal/flowkit/install.gointernal/flowkit/scripts_github.gointernal/flowkit/scripts_local.gointernal/store/builtin.gointernal/tools/schemagen/main.gotest/integration/flow_drawer_test.gotest/integration/flow_recovery_test.gotest/integration/flow_runtime_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/drawer/compile_flow_test.go
- test/integration/flow_runtime_test.go
- internal/drawer/compile_flow.go
- internal/drawer/entity.go
- internal/flowkit/install.go
- internal/flowkit/scripts_local.go
- test/integration/flow_drawer_test.go
Comment on lines
+20
to
+29
| // Create the scaffold (directory + AGENTS.md + pressed/). | ||
| if _, err := svc.CreateWithKind("research-deck", "Research a topic and create an open-slide deck (https://open-slide.dev/).", nil, drawer.DrawerKindFlow); err != nil { | ||
| return err | ||
| } | ||
| // Overwrite with the canonical definition. | ||
| d := drawer.ResearchDeckDrawer(provider) | ||
| now := time.Now().UTC() | ||
| d.CreatedAt = now | ||
| d.UpdatedAt = now | ||
| return svc.Save(d) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make research-deck installation atomic.
CreateWithKind persists an empty flow scaffold before svc.Save(d) writes the canonical stages. If line 29 fails, the incomplete drawer remains on disk. A later call returns at line 17 because svc.Get("research-deck") succeeds, so it never repairs the drawer.
Remove the newly created scaffold when the canonical save fails, or add a service operation that creates the canonical drawer in one transaction.
🤖 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 `@cmd/flow_research_deck.go` around lines 20 - 29, Make the research-deck
installation atomic by updating the flow around CreateWithKind and Save in the
research-deck installation function: if saving the canonical ResearchDeckDrawer
fails, remove the newly created scaffold before returning the error, or use an
existing service operation that persists the canonical drawer transactionally.
Preserve the existing duplicate-installation check and successful scaffold
creation behavior.
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
drawer_kind: "flow"pressable by compiling the flow definition in memory at press time into the existing drawer executor pipeline (provider-list → claim → perform → validate → apply → ensure-trigger), wired for bothdrawer pressand webhook dispatch.buttons flowCLI sugar (init,task,status,logs,approve/reject,rm).@buttonsflow/research-deck(research → open-slide deck) with claim/staleness recovery and integration coverage.Test plan
go test ./internal/drawer/ ./internal/flowkit/ ./internal/store/ ./cmd/go test ./test/integration/ -run 'Flow|ResearchDeck|ButtonsFlow'buttons add @buttonsflow/research-deck && buttons flow init research-deckbuttons flow task add research-deck "Q3 competitive landscape for leadership offsite"buttons flow approve, confirmbuttons flow task list research-deck --filter status=donestepson diskMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes