Introduces in-memory approval queue system to support interactive plan approval workflows#41
Open
AkshatRaj00 wants to merge 4 commits into
Open
Introduces in-memory approval queue system to support interactive plan approval workflows#41AkshatRaj00 wants to merge 4 commits into
AkshatRaj00 wants to merge 4 commits into
Conversation
…endpoint stubs (#8) - Add PlanEditRequest, PlanApprovalAction, ApprovalQueueEntry types to src/types/index.ts - Add src/core/plan-approval.ts: approval queue manager with enqueue/dequeue/update - Add REST endpoint stubs in src/ui/plan-editor-routes.ts for GET/PATCH/POST plan approval - Closes #8 (partial — MVP foundation for interactive plan editor dashboard flow)"
feat(ui): plan editor approval queue — MVP foundation for Issue #8
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an in-memory Approval Queue Manager and associated REST endpoints to handle plan approvals, step edits, and reviewer decisions. Key feedback includes a critical issue where Express-specific APIs were used despite the project employing a custom HTTP server, which will cause runtime errors. Additionally, improvements were suggested for applyPlanEdit to distinguish between 'not found' and 'invalid state' errors and to avoid redundant metadata updates. Finally, the reviewer recommended implementing state transition validation in recordDecision to prevent race conditions and ensure data consistency.
Owner
|
pls ensure CI passes. next time pls also write a good title for the PR and good PR body... |
hoangsonww
added
bug
github-project-automation
Bot
moved this to Backlog
in Forge Local-First Agentic Coding Tool Kanban Board
hoangsonww
changed the title
- plan-editor-routes.ts: replace Express-based handlers with native http.IncomingMessage + http.ServerResponse to fix TS7016 (@types/express not installed). Removes all no-non-null-assertion and no-explicit-any ESLint violations that came from Express typings. - plan-approval.ts: add discriminated result types (PlanEditResult, DecisionResult) for applyPlanEdit and recordDecision — eliminates implicit null returns that caused lint warnings. Fix Prettier formatting. - Both files now pass: prettier --check, tsc --noEmit, eslint Closes format / typecheck / lint / pipeline-status CI failures on PR #41
- applyPlanEdit and recordDecision now return { ok, entry } | { ok, reason }
instead of ApprovalQueueEntry | null — update all test assertions to
unwrap .entry and check .ok instead of null comparisons / non-null !
- Removes all non-null assertion (!) operators from test file which were
causing @typescript-eslint/no-non-null-assertion lint errors
- plan-editor-routes.ts: fix lint — remove unused 'URL' import (already
available as global in Node 10+), fix no-unused-vars warning
Fixes: lint / test (ubuntu node 20+22, macos node 20+22) CI failures
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
2 participants
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.
This pull request introduces an in-memory approval queue system to support interactive plan approval workflows, along with REST API endpoints and comprehensive unit tests. The main changes add a queue manager for tracking and updating plan approval entries, expose REST endpoints for UI integration, and provide tests to ensure correct behavior.
Core plan approval queue implementation:
src/core/plan-approval.tscontaining the approval queue manager, including types for approval status, queue entries, plan edit requests, and actions. Exposes functions to enqueue, list, retrieve, edit, approve/reject, and dequeue plan entries, all managed in-memory for MVP purposes.REST API endpoints:
src/ui/plan-editor-routes.tswith Express route handlers to list all queue entries, fetch a single entry, apply plan edits, and record approval decisions. These endpoints are designed for easy integration with the UI and return consistent JSON responses.Testing:
test/plan-approval.test.tswith unit tests covering all major queue operations: enqueue, list, edit, approve/reject, and dequeue. Ensures correct queue behavior and edge case handling.