Replace in-progress emoji with review outcome - #133
Merged
Merged
Conversation
Introduce `done_emoji` and `fail_emoji` to visually track review status. The daemon now awards a start emoji when a review begins, then replaces it with the appropriate outcome emoji once the review lands or fails. Command notes are similarly acknowledged and updated. Key changes: - Add `done_emoji` and `fail_emoji` configuration options with validation to prevent conflicts with trigger or start emojis. - Implement `ReplaceMREmoji` and `ReplaceNoteEmoji` in the GitLab client to atomically revoke old reactions and award new ones. - Update the worker to track which reactions were placed during a review and settle them with the final outcome. - Improve command parsing to skip leading blank lines in comments. - Add test utilities to clear ambient environment variables for CI consistency.
The name comes from os.Environ, so it is always valid and the only error Unsetenv can report cannot happen here. Explicitly ignore the return value to satisfy linter requirements.
Revoking by name alone (unresolved own user id) is refused: an administrator or owner token CAN delete another user's award, so the name-only fallback could revoke a human's genuine reaction. The add half of a replace still goes through. awardEmoji no longer swallows 401/403/429 as "already awarded": those mean the award never happened (lost token access, rate limit), and treating them as success let a replace revoke successfully and drop the new reaction in silence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ment gaps Reaction settlement lived only inside process(), so several job-disposal paths stranded the in-progress ack on command notes forever. Now: - Abort releases the acks of a queued job and of a cleared pending re-run; only the running review settles its own notes. - Shutdown no longer strands accepted jobs: with a state_dir configured they are journaled (one JSON file per MR, no tokens) and resumed at the next start; without one their acks are revoked on the way out. - Notes dropped over the per-job ack cap are released at merge time. - The review ack is awarded synchronously BEFORE the job is visible to a worker (bounded to 5s), so an immediately-settling review can never be outrun by its own acknowledgement; a rejected enqueue revokes it. - An abort landing during the pre-run MR status check is classified as aborted, not failed. - Settle flips run concurrently, each on its own 15s deadline (matching the helm comment), and also remove stale done/fail outcomes a previous run left behind. - The handler reads the ack emoji from the dispatcher instead of a second plumbed copy. Config: new state_dir setting / --state-dir flag; a DEFAULTED done/fail emoji colliding with an explicitly configured reaction is disabled with a startup notice instead of failing validation, so pre-outcome-emoji configs (e.g. ack_emoji: "white_check_mark") keep starting after an upgrade. The five LoadServe nil-default blocks collapse into the emojiOrDefault getter helper. Helm: serve.stateDir (default /work/state) rendered into server.yaml; optional persistence.* PVC mounted at that path so the journal survives pod replacement, not just container restarts. Tests: fakes attribute awards to a resolved bot user id, "must not decorate" assertions use the permanent award-POST log instead of the live view, and TestDispatcherRunsReview waits for settle instead of racing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delete `ClearAmbientEnv` helper and its usages in `TestMain` for CLI and config packages. This removes the mechanism that unsets `NICKPIT_*`, `_API_KEY`, and `_TOKEN` variables before tests run, likely as part of a broader shift in how test fixtures or configuration isolation is handled.
The dispatcher now tracks whether each job was successfully persisted to the journal. This ensures that unfinished jobs are correctly handled during shutdown: - Persisted jobs are resumed on restart. - Jobs that failed to persist (e.g., due to disk full or read-only errors) have their acknowledgements released immediately to prevent them from being stuck in progress forever. The journal's `persist` method now returns a boolean indicating success, allowing the dispatcher to make informed decisions about durability.
Require the bot user ID to be resolved before allowing emoji operations. Without it, name-based revocation is unsafe as admin tokens can delete other users' awards, leading to contradictory reactions. Startup now fails if the identity cannot be determined. Track configured emoji names in the journal and event state to ensure clean removal of markers across config changes or restarts. Failed journal updates now invalidate stale snapshots to prevent resuming inconsistent state.
Track whether a restored job previously attempted a start-reaction request. This ensures that crash recovery metadata does not cause a live worker to decorate an MR it rejects before attempting the reaction.
Update the emoji logic to confirm new awards before revoking old ones, preventing silent data loss if the new award fails. Refine duplicate award detection to parse GitLab's response body rather than relying on inconsistent HTTP status codes across versions. Bump Helm chart version to 0.5.0.
Add a hidden `--require-publish` flag to the CLI that forces a non-zero exit code if publishing the review fails. This allows the serve daemon to distinguish between successful reviews with delivery warnings and actual failures. The runner automatically enables this flag for serve children to ensure reliable outcome reporting. Refactor GitLab emoji handling to use bot-owned reaction cleanup. Replace name-based revocation with methods that clear all awards owned by the bot, preserving only the trigger emoji. This prevents stale outcome markers from persisting after configuration changes or restarts. Update Helm chart to version 0.5.1 and omit default emoji values from rendered configs to maintain upgrade compatibility.
Introduce a bounded worker pool for acknowledgement cleanup to prevent unbounded goroutine growth during webhook floods. The cleanup queue and worker count are now limited, with excess requests logged and discarded. Move the persistence of the start reaction name from `take` to immediately before the remote API request. This ensures that restored jobs only show the start marker if the previous process actually reached the GitLab award endpoint, preventing false positives after crashes during policy checks. Add Helm validation to enforce absolute paths for `serve.stateDir` when persistence is enabled. Update documentation and tests to reflect these behavioral changes.
Introduce a new `stateCleanup` phase to handle remote reaction outcomes that must survive process restarts or transient failures. This ensures that emoji replacements (start/ack) are reliably applied even if the review process is aborted or crashes mid-execution. Key changes: - Add `reactionCleanup` state to track durable reaction work separately from active reviews. - Persist cleanup state in the journal when a review finishes with a settlement result, fails, or is aborted. - Implement `queueDurableCleanup` with exponential backoff and version checks to prevent duplicate retries after restarts. - Update `settle` to return a boolean indicating success, allowing the dispatcher to distinguish between successful and failed settlements. - Refactor `finish` and `Abort` to transition jobs into the cleanup state rather than immediately removing them. - Add tests for settlement failures, abort cleanup restoration, and journal durability during crashes.
Distinguish between 403 (permission) and 404 (gone) errors when revoking GitLab emoji reactions. A 403 indicates the bot lacks permission to delete a reaction it previously added, so it must not be silently ignored. Introduce support for "aborted" events in the dispatcher and journal. When a pending review is cancelled while cleanup is blocked, its acknowledgements are tracked separately and revoked on shutdown or retry without applying the original outcome. Refactor the settle logic to return remaining unresolved reactions instead of a boolean. This enables selective retries for transient failures while treating 404s as terminal. Add a grace period timeout to `stopAckCleanup` to prevent shutdown hangs during high load.
When GitLab rejects an invalid or unsupported outcome emoji (400/422), the worker now detects this via `reactionOutcomeRejected`. Instead of retrying indefinitely or leaving in-progress markers, it degrades to a revoke-only cleanup to retire the target. Additionally, the dispatcher ensures that new events arriving during an abort's finish window are queued as pending behind the durable cleanup state, preventing journal corruption after a crash.
Introduce SettleMR and RevokeMROnly fields to track whether a merge request has been touched and if cleanup should be revoke-only. This ensures that empty start emojis correctly trigger revocation of old markers without adding outcome reactions, preserving disabled-decoration semantics across restarts and configuration changes. Replace ad-hoc checks on StartEmojis with the eventSettlesMR helper for consistent behavior. Enforce queue capacity limits on non-running cleanup states to prevent unbounded growth during outages. Update journal loading to use safe directory iteration and improve logging for emoji operations.
Ensure crash snapshots transition to cleanup-only state before remote reaction settlement begins. This prevents a replacement daemon from re-running and republishing a completed review if a crash occurs in the I/O window between job completion and reaction removal. Add Helm validation to prevent persistence configuration conflicts with fixed mount paths.
…haustion The `releaseUnfinished` function previously allowed unbounded fan-out of GitLab requests during shutdown, potentially exceeding system limits. This change introduces a bounded worker pool and a reaction slot mechanism to cap live remote requests. Key changes: - Add `settleWithLimit`, `releaseAcksWithLimit`, and `settleReactionCleanupWithLimit` functions that accept an optional slot channel for concurrency control. - Refactor `releaseUnfinished` to use a bounded channel of workers and a shared `reactionSlots` semaphore. - Implement `withReactionSlot` helper to acquire/release slots with context cancellation support. - Add test `TestDispatcherShutdownReactionCleanupIsBounded` to verify the concurrency cap is respected during journal-less shutdown.
Remove the wrapper function releaseAcks and update the comment for releaseAcksWithLimit to reflect its current scope of revoking acks for notes discarded at shutdown.
…ience Pass a context with timeout to `releaseUnfinished` and `stopAckCleanup` to ensure a shared deadline for all cleanup operations during shutdown. This prevents the total shutdown time from multiplying across batches of pending requests, ensuring the process respects the configured grace period. Update `Journal.load` to distinguish between read failures and malformed entries. Files that fail to read are now preserved for retry on the next startup, while only successfully read but malformed files are removed. This improves resilience against transient filesystem issues.
Track acknowledgements that timed out on the client side as uncertain, ensuring they are swept later if GitLab commits them after the initial outcome. Add UncertainAckNoteIDs and AckCleanupUntil to the Event struct and journal entries to persist this state across restarts. Update dispatcher to merge and clean up uncertain awards during settlement and shutdown, reserving budget for unfinished jobs. Adjust the handler to detect request timeouts and enqueue delayed cleanup tasks.
The `APIError` struct now includes a `RetryAfter` time field populated from HTTP response headers (`Retry-After` or `RateLimit-Reset`). This allows callers to respect server-side backoff instructions. Refactor dispatcher to share retry pacing across the entire cleanup backlog. Instead of each job retrying independently, a bounded wave admits jobs through a shared outage gate with exponential backoff. Failed attempts update a global `cleanupRetryUntil` deadline, preventing thundering herds during remote outages. Add support for uncertain acknowledgement timeouts. Cleanup jobs for notes that may have timed out are delayed until the uncertainty window closes. Shutdown logic preserves these pending operations in a fallback queue and extends the grace period if necessary to ensure final revocations occur after late POSTs commit. Introduce `reactionSettlement` to distinguish between transient failures and known timeouts. This enables correct handling of mixed success/failure scenarios within a single settlement attempt.
Prevent command injection vulnerabilities by validating that review extra arguments do not contain the end-of-options marker "--". Also, refactor reaction cleanup logic to extract uncertainty deadline calculation into a dedicated helper. This ensures durable cleanup correctly waits for final sweeps of aborted events with uncertain acknowledgements.
Simplify argument validation in ExecRunner.Run by using the standard library's slices.Contains function to check for "--" in ExtraArgs, reducing boilerplate code.
…erruptions Introduce `StartCleanupUntil` to track deadlines for final sweeps after start-reaction requests return cancellation or timeout errors. GitLab may commit delayed awards that were not detected during immediate settlement, requiring a later cleanup pass. Refactor uncertainty handling to be generic across all reaction types, renaming internal functions from `Ack` to `Reaction`. Add `outcomeInterrupted` to distinguish root shutdown cancellations from user aborts; interrupted reviews are preserved in the journal as runnable state to resume upon restart rather than marking them as failures. Update handler logic to queue acknowledgement cleanup even when the review queue is full, ensuring late-arriving acknowledgements are swept correctly.
Enforce that the state directory is not writable by group or other users to prevent local users from injecting work via predictable paths. Use `os.CreateTemp` with randomized names for journal updates to avoid symlink attacks, and ensure directory syncs are persisted. Improve cleanup backoff logic to correctly preserve longer server-side `Retry-After` deadlines when concurrent attempts finish out of order.
- Mount PVC at `serve.stateDir` parent and use a private `journal/` subdirectory to avoid issues with group-writable root permissions. - Add platform-specific ownership validation for the journal directory. - Implement tombstone-based retirement with retry logic to prevent re-running completed jobs after crashes or unlink failures. - Update Helm chart values, templates, and documentation accordingly.
Use `os.OpenRoot` to pin the validated state directory, preventing symlink attacks or path replacement during runtime. Replace string-based directory operations with methods on the pinned root handle. Refactor retirement logic to use a bounded channel and dedicated worker goroutine instead of spawning per-job goroutines. This caps memory usage and filesystem work during storage outages. Add proper lifecycle management with `Close` and `stopRetirements` for clean shutdown.
Use platform-specific descriptor-relative walks to open the journal directory without following symlinks. This prevents a writable ancestor from redirecting traversal between a pathname check and the open, mitigating TOCTOU race conditions. Update documentation to clarify that the state directory must be owned by the daemon user. Adjust Docker Compose example to include an init container for creating the journal directory with correct ownership. Set Helm fsGroupChangePolicy to OnRootMismatch to avoid unnecessary reapplying of permissions on PVC remounts.
Replace strings.Split with strings.SplitSeq to iterate over path components. This avoids allocating a slice and improves performance when processing journal paths.
- Refactor `runner.go` to reject `--`, `--help`, and `-h` in extra args with specific error messages instead of a generic check. - Update `worker.go` to prioritize successful execution over late root cancellation during log drain, ensuring durable jobs are retired. - Add test for successful run winning over shutdown during log drain. - Add early returns to tests to prevent panics on nil assertions.
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.
Introduce
done_emojiandfail_emojito visually track review status.The daemon now awards a start emoji when a review begins, then replaces it
with the appropriate outcome emoji once the review lands or fails. Command
notes are similarly acknowledged and updated.
Key changes:
done_emojiandfail_emojiconfiguration options with validationto prevent conflicts with trigger or start emojis.
ReplaceMREmojiandReplaceNoteEmojiin the GitLab clientto atomically revoke old reactions and award new ones.
and settle them with the final outcome.
consistency.