fix(gastown): remove dead hasExistingFeedbackBead and add now() helper to reconciler#2028
Open
jrf0110 wants to merge 16 commits intoconvoy/fix-reconciler-p0-p1-bug-fixes-from-audi/892c7c4a/gt/toast/dd3b7f14from
Conversation
- docs/local-debug-testing.md: comprehensive guide covering debug endpoints, container monitoring, drain testing, wrangler management, drain architecture, and common issues - scripts/test-drain.sh: automated end-to-end drain test that sends work, waits for an agent, triggers graceful stop, monitors the drain, and prints PASS/FAIL with diagnostics
…#1979) fix(gastown): also refresh container token when mayor is alive (waiting) Refine the refreshContainerToken guard to check for an alive mayor (working/stalled/waiting) in addition to hasActiveWork(). A waiting mayor is alive in the container but hasActiveWork() returns false — without this fix the 8h container token can expire, stranding GT tool calls when the user sends the next message via sendMayorMessage (which reuses the container without calling ensureContainerToken). Co-authored-by: John Fawcett <john@kilcoode.ai>
* feat(gastown): skip review queue for gt:pr-fixup beads (#1983) * feat(gastown): skip review queue for gt:pr-fixup beads in agentDone() * style: fix formatting in local-debug-testing.md --------- Co-authored-by: John Fawcett <john@kilcoode.ai> * feat(gastown): thread labels parameter through gt_sling tool chain Add optional labels[] parameter to gt_sling so the mayor can tag beads at creation time. The parameter flows through: mayor-tools.ts → client.ts → mayor-tools.handler.ts → Town.do.ts slingBead() → createBead() createBead() already accepts labels, so no further changes needed. * feat(gastown): add pr_fixup_context to PrimeContext and buildPrimeContext Add pr_fixup_context field to PrimeContext type and populate it in buildPrimeContext when the hooked bead has the gt:pr-fixup label, mirroring the existing rework_context pattern. Extracts pr_url, branch, and target_branch from bead metadata. --------- Co-authored-by: John Fawcett <john@kilcoode.ai>
…dev libraries (#1978) * feat(container): expand apt-get block with build tools, ripgrep, and dev libraries Add comprehensive package list to both Dockerfile and Dockerfile.dev: - Build toolchain: build-essential, autoconf - Search tools: ripgrep, jq - Network: wget, gnupg, unzip - Compression: bzip2, zstd - SSL/crypto: libssl-dev, libffi-dev - Database client libs: libdb-dev, libgdbm-dev, libgdbm6 - Python build deps: libbz2-dev, liblzma-dev, libncurses5-dev, libreadline-dev, zlib1g-dev - Ruby build deps: libyaml-dev - Image processing: libvips-dev - Browser/rendering: libgbm1 - Native addon support: libc++1, libgmp-dev - Timezone data: tzdata Closes #1976 * fix(container): remove shell comments from inside backslash-continued apt-get install Shell comments inside backslash-continued lines break the apt-get install command — the # starts a comment that swallows the rest of the logical line, dropping all subsequent packages. Move category info to a block comment above the RUN statement instead. --------- Co-authored-by: John Fawcett <john@kilcoode.ai>
…_review toggle (#1586) * feat(gastown): auto-resolve PR feedback and auto-merge with grace period Implements the core infrastructure for automatically addressing unresolved review comments and failing CI checks on PRs, plus configurable auto-merge with a grace period timer. Closes #1002 * fix(gastown): address PR review feedback — pagination safety, merge failure recovery, repo validation - GraphQL reviewThreads query now includes pageInfo.hasNextPage; if there are more than 100 threads, conservatively treats PR as having unresolved comments to prevent auto-merging with unchecked feedback - merge_pr action clears auto_merge_pending flag on failure (405/409/error) and resets the auto_merge_ready_since timer so normal polling resumes - reconcileReviewQueue now emits poll_pr alongside merge_pr so PR status changes (merged/closed by human) are still observed during merge attempts - merge_pr validates the PR URL's owner/repo against the rig's git_url before calling the merge API, preventing merges targeting unrelated repos * fix(gastown): check-runs pagination and fresh feedback check before merge - check-runs API now uses per_page=100 and compares total_count to detect unpaginated runs; if more runs exist than returned, allChecksPass is conservatively set to false - merge_pr re-checks feedback (comments + CI) immediately before calling the merge API; if state regressed since the last poll, clears auto_merge_pending and resets the timer * fix(gastown): conservative hasFailingChecks for unpaginated check-runs, fix CI failures - hasFailingChecks is now conservatively true when total_count > returned runs, so PRs with >100 check-runs correctly trigger feedback handling - Replace `as` casts with Zod safeParse for GitHub API responses (checkPRFeedback) to satisfy oxlint no-unnecessary-type-assertion - Fix test constant expressions flagged by TS2871 and oxlint - Run oxfmt on changed files * fix(gastown): separate hasUncheckedRuns from hasFailingChecks in PR feedback hasFailingChecks now only reflects actual failures from inspected runs. A new hasUncheckedRuns field tracks when the check-runs response was paginated. allChecksPass is still conservatively false when runs are unchecked (blocking auto-merge), but the feedback bead is no longer dispatched for phantom CI failures on repos with >100 passing checks. * style: auto-format local-debug-testing.md * feat(gastown): polecat creates PRs, refinery reviews via GitHub, code_review toggle - Polecat creates PRs directly when merge_strategy is 'pr', passing pr_url to gt_done. Removes a full agent dispatch cycle from the flow. - Refinery reviews existing PRs via gh pr review, adding inline comments that go through the same auto-resolve feedback loop as human reviews. - New refinery.code_review toggle (default: true) lets users disable refinery code review when they have an external review bot. - Fix: allChecksPass treats 0 check-runs as passing for repos without CI. - Fix: reconciler Rule 7 stops refineries hooked to terminal MR beads, preventing post-merge review comments. - Convoy-aware: review-then-land intermediate beads skip PR creation. * fix(gastown): include hasUncheckedRuns in pr_feedback_detected condition When check-runs are paginated (>100), the first page may all pass but later pages may have failures. Previously only hasFailingChecks was checked, which only reflects the first page. Now hasUncheckedRuns also triggers feedback detection so the auto-resolve path can fix CI. * fix(gastown): include auto_merge columns in REVIEW_JOIN query REVIEW_JOIN was missing auto_merge_ready_since and last_feedback_check_at from the SELECT, causing MergeRequestBeadRecord parse failures on pr_status_changed events. * fix(gastown): refinery uses gh pr comment instead of --approve The bot account that created the PR cannot approve its own PRs. The refinery now posts a comment when the review passes instead of using gh pr review --approve. * fix(gastown): set parent_bead_id on rework and PR feedback beads Rework beads (from refinery review) and PR feedback beads (from poll_pr comment detection) now set parent_bead_id to the MR bead that triggered them. This links them into the bead chain so the relationship is visible in the UI and queryable via parent traversal. * fix(gastown): skip orphaned-review timeout when code_review disabled, fix inline comment syntax - Rule 4 orphaned-review timeout now skips when refineryCodeReview is false — poll_pr keeps MR beads alive, no refinery expected. - Removed invalid 'gh pr review --comment path:line' syntax from refinery prompt; consolidated on the GitHub API example which correctly creates inline review threads.
… auto-merge on toggle - Also check combined commit status API alongside check-runs; repos using legacy status checks are no longer missed. - Propagate hasUncheckedRuns through pr_feedback_detected event so the feedback bead title and prompt reflect the actual reason. - Gate auto-merge timer on refineryConfig.auto_merge in addition to auto_merge_delay_minutes to respect the toggle.
…, stuck MR beads (#1632), session leak (#1341) * fix(gastown): preserve org billing context across model changes (#1756) Store organizationId as a standalone env var (GASTOWN_ORGANIZATION_ID) that survives KILO_CONFIG_CONTENT rebuilds. Three defense layers: 1. Set GASTOWN_ORGANIZATION_ID on /agents/start and read it first in extractOrganizationId() before falling back to config extraction. 2. Pass organizationId in the PATCH /model request body so the container receives it even if X-Town-Config parsing fails. 3. Include organization_id in buildContainerConfig and sync it to process.env in the PATCH handler's X-Town-Config processing. * fix(gastown): stop false-positive invariant violations spamming logs every 5s Invariant 7: Exclude mayors from the 'working agent must have a hook' check — mayors are always working and intentionally hookless. Invariant 5: Widen valid convoy bead states to include in_progress and in_review, which are legitimate transient states while child beads are being worked on. Only flag truly unexpected states. Fixes #1364 * fix(gastown): prevent MR beads stuck in_progress when github_token missing (#1632) - Fall back to github_cli_pat then platform integration token in checkPRStatus - Track consecutive null poll results; fail MR bead after 10 nulls - Rate-limit PR polling to once per minute per MR bead via last_poll_at metadata - Store failureReason and failureMessage in bead metadata on permanent failure * fix: format and lint errors in reconciler.ts - Add explicit 'unknown' type annotation to lastPollAt to fix no-unsafe-assignment lint error - Auto-format ternary expression per oxfmt rules * fix(gastown): prevent duplicate session leak when restarting a starting agent Thread an AbortController through the startAgent() startup sequence so that when a restart is requested for an agent still in 'starting' status, the in-flight startup is cancelled before session.create() can produce an orphaned session. - Add startupAbortController field to ManagedAgent type - In startAgent: abort existing startup controller when agent is 'starting' - Check signal.aborted after each async step (ensureSDKServer, session.create, before session.prompt) - On abort: decrement sessionCount, remove agent entry, clean up SDK instance if no sessions remain - In stopAgent: also abort startupAbortController for 'starting' agents - Introduce StartupAbortedError to distinguish abort from real failures Fixes #1341 * fix(gastown): address review comments — reset poll_null_count on all non-null polls, abort orphaned session on startup abort, guard agents.delete with identity check * fix(gastown): store session ID before abort check to prevent orphaned session leak After session.create() resolves, parse and store the session ID on the agent object BEFORE checking signal.aborted. This ensures the catch block has agent.sessionId available to call session.abort(), closing the race window where an abort during session.create() could leak an orphaned session. Also fixes format-check CI by running oxfmt.
…ct in gt_sling/gt_escalate (#1996) * feat(gastown): add PR-fixup workflow to polecat and mayor system prompts (#1989) * feat(gastown): add PR-fixup workflow to polecat and mayor system prompts Add instructions for the gt:pr-fixup workflow: - Polecat prompt: how to handle PR fixup beads (check out branch, address review comments, resolve threads, push to existing branch) - Mayor prompt: how to dispatch PR fixup beads using gt_sling with the gt:pr-fixup label and appropriate metadata * WIP: container eviction save * fix: address PR #1989 review comments — metadata type and branch conflict - Remove JSON.stringify wrapper from metadata example in mayor prompt so it passes gt_sling's z.record(z.string(), z.unknown()) validation as a plain object. - Clarify polecat PR fixup workflow as the explicit exception to the 'do not switch branches' rule, and add exception note in Commit & Push Hygiene section. --------- Co-authored-by: John Fawcett <john@kilcoode.ai> * fix(gastown): gt_sling and gt_escalate accept metadata as object instead of JSON string The metadata parameter on gt_sling (mayor) and gt_escalate (polecat) now uses z.record(z.string(), z.unknown()) so the LLM can pass a plain object directly. Removes the parseJsonObject indirection and dead code. Updates tests to match. --------- Co-authored-by: John Fawcett <john@kilcoode.ai>
New debug endpoint returns the full bead record, review_metadata, and bead_dependencies for a given bead ID. Useful for verifying parent_bead_id linkage, pr_url storage, and auto_merge_ready_since timer state during local testing.
…ainer networking tips - Document the new debug/beads/:beadId endpoint for bead inspection - Add Test C: code_review toggle verification procedure - Add bead chain verification examples - Document container networking workaround for local dev
…token fallback, guard debug endpoint - Clear GASTOWN_ORGANIZATION_ID when org_id is absent (prevents stale billing context when a town moves from org to personal). - Change poll failure reason from 'no_github_token' to 'pr_poll_failed' with a message that covers all null-poll causes (token, rate limits, 5xx). - checkPRFeedback and mergePR now use the same token fallback chain as checkPRStatus (github_token → github_cli_pat). - Guard debug/beads/:beadId endpoint with dev-only check.
Swap Switch and label/description div positions in all 5 toggle cards on the town settings page so toggles are right-aligned.
… across all PR operations All three GitHub PR operations (checkPRStatus, checkPRFeedback, mergePR) now use the same resolveGitHubToken helper with the full fallback chain: github_token → github_cli_pat → platform integration (GitHub App). Towns relying only on a GitHub App installation token will now have auto-resolve and auto-merge working correctly.
…re on merge - Rule 5 now includes MR beads with rig_id IS NULL (uses COALESCE sentinel). For NULL-rig beads, any idle refinery is eligible. Previously these beads were silently skipped, leaving them stuck in 'open' forever. - completeReviewWithResult only closes the source bead if it's in 'in_review' state. Prevents stale MR beads (from pre-deploy PRs already merged on GitHub) from incorrectly closing source beads that are still open/in_progress.
…r to reconciler - Remove hasExistingFeedbackBead from actions.ts since dedup moved to reconciler's pr_feedback_detected event handler (hasExistingPrFeedbackBead) - Remove redundant dedup guard in poll_pr; reconciler handles it synchronously - Add local now() helper to reconciler.ts for timestamp consistency - Replace raw new Date().toISOString() call with now()
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ); | ||
| } | ||
| } | ||
| instance.sessionCount--; |
Contributor
There was a problem hiding this comment.
WARNING: sessionCount can be decremented twice on startup abort
startAgent() now cancels a starting agent by calling stopAgent(), and stopAgent() already aborts the SDK session and decrements instance.sessionCount when a session exists. If the old startup then reaches this StartupAbortedError cleanup after session.create() resolved, this block decrements the same counter again and can close the shared SDK server while another session is still using it.
| } | ||
|
|
||
| // Update org billing context from the request body if provided. | ||
| if (parsed.data.organizationId) { |
Contributor
There was a problem hiding this comment.
WARNING: Hot-swap fallback cannot clear a stale organization context
This truthy check means the model-update request can only set GASTOWN_ORGANIZATION_ID, never clear it. On an org -> personal transition, if X-Town-Config parsing fails (the exact case this request-body fallback is meant to cover), the container keeps the previous org ID and later model calls keep billing against the old organization. Mirroring /agents/start here with parsed.data.organizationId ?? '' would preserve the fallback behavior.
Contributor
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (35 files)
Reviewed by gpt-5.4-20260305 · 1,133,277 tokens |
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
hasExistingFeedbackBeadfunction fromactions.ts— dedup logic now lives in the reconciler's synchronouspr_feedback_detectedevent handler (hasExistingPrFeedbackBead)poll_pr's feedback detection; the event is now emitted unconditionally and the reconciler handles idempotencynow()helper toreconciler.tsand replaced the single rawnew Date().toISOString()call for consistency with the rest of the codebaseVerification
pnpm typecheckpasses with 0 errorsbead_dependenciesimport verified still in use at lines 420-423 (tracks dependency query), so it was keptVisual Changes
N/A
Reviewer Notes
hasExistingFeedbackBeadcall at line 644 inpoll_prwas the only call site. Since thepr_feedback_detectedevent handler inreconciler.ts(line 384) already callshasExistingPrFeedbackBeadbefore creating a feedback bead, the dedup check inpoll_prwas redundant.new Date().toISOString()instance was found inreconciler.ts(at thepr_auto_mergeevent handler), not four as the feedback mentioned — the other occurrences may have been in an earlier revision.