Skip to content

[AISOS-2294] Add decompose draft review step before Jira task creation - #242

Open
ekuris-redhat wants to merge 64 commits into
forge-sdlc:mainfrom
ekuris-redhat:forge/aisos-2294
Open

[AISOS-2294] Add decompose draft review step before Jira task creation#242
ekuris-redhat wants to merge 64 commits into
forge-sdlc:mainfrom
ekuris-redhat:forge/aisos-2294

Conversation

@ekuris-redhat

@ekuris-redhat ekuris-redhat commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

This pull request implements an interactive, draft-based planning review flow for epic decomposition and task generation workflows. It introduces Pydantic models for draft representation, robust Jira attachment helpers and management utilities, comment commands and natural language feedback revision logic, and integrates a State Consistency Guard with rollback capabilities in the orchestrator worker. Ultimately, this allows human-in-the-loop validation, modification, and direct provisioning of generated tickets from structured draft JSONs attached to Jira issues.

Changes

Models & Schema Verification

  • Created src/forge/models/draft.py defining DraftItem (supporting an epic_key field to correctly map Tasks to parent Epics) and ForgeDecompositionDraft Pydantic v2 models, enforcing unique sequential item IDs and exclusion flags (via an excluded boolean property).
  • Exported the draft models in src/forge/models/__init__.py.

Jira Integrations & Utilities

  • Modified src/forge/integrations/jira/client.py to add robust Jira attachment helpers (get_attachments—which merges attachment listing and metadata fetching, download_attachment, delete_attachment, delete_attachments_by_name, edit_comment with robust retry logic, and add_attachment supporting dynamic content types) with automatic rate-limit and retry logic, updated the deprecated Jira Search Endpoint to POST /search/jql with queries scoped via issuetype checks to prevent collisions during task provisioning, and resolved a concurrent uploading race condition.
  • Created src/forge/workflow/utils/draft_manager.py implementing DraftManager to handle JSON draft serialization, secure downloading, deserialization, single-draft attachment constraints, and transactional in-memory draft modifications (add, update, remove, exclude) while preserving table alignment by escaping pipe (|) characters, and adding strikethrough formatting and *(excluded)* visual indicators to excluded items in draft reviews.

Comment Command & Natural Language LLM Parsing

  • Enhanced src/forge/workflow/utils/comment_classifier.py by adding CommentType.COMMAND and introducing regex-based comment command classification.
  • Implemented parse_comment_command to extract operations and arguments for /forge add, /forge update, /forge remove, /forge exclude, and /forge approve.
  • Created prompt template src/forge/prompts/v1/revision-draft.md to feed draft edits and natural language feedback into an LLM chain.
  • Added revise_draft_with_feedback in src/forge/integrations/agents/agent.py to revise draft structures using LLM feedback with a clean, Markdown-stripping JSON parsing utility featuring delimiter-matched boundary extraction (safely matching start and end delimiters to prevent parsing failures when trailing mismatched punctuation is present), sanitizing ValueError exceptions to prevent prompt internals leakage.

Orchestration & State Machine Integration

  • Modified src/forge/workflow/nodes/epic_decomposition.py and src/forge/workflow/nodes/task_generation.py to conditionally bypass or enter the draft review gate (pausing at PENDING_APPROVAL, supporting direct ticket creation mode without full YOLO mode via the forge:direct-mode label, slicing task drafts per Epic to attach localized task drafts to Epic tickets, ensuring zero-item counts trigger retries across all paths (YOLO/non-YOLO/direct-mode), and writing markdown previews with a 32,767 character/15-item truncation threshold).
  • Modified src/forge/orchestrator/worker.py to capture comments while paused, applying either /forge mutation commands or leading whitespace-tolerant ! natural language revisions on attached draft JSONs.
  • Implemented State Consistency Guard (BR-006) in the worker to automatically roll back to the previously attached draft version and report failures (with redacted secrets) upon any processing errors.
  • Moved ticket provisioning logic out of conditional edge routers and into dedicated standard graph nodes (provision_epics, provision_tasks), integrating JQL-based Jira-side idempotency guards in plan and task approval.
  • Integrated automated Jira ticket provisioning upon approval comments (/forge approve) or transition label events (forge:plan-approved, forge:task-approved) inside the orchestrator worker, skipping excluded items and ensuring attachment deletion only after successful creation of all items.

Documentation Updates

  • Updated CLAUDE.md, docs/guide/labels.md, docs/guide/feature-workflow.md, and docs/developer-guide.md to reflect interactive comment commands, workflow diagrams, and state-machine characteristics.

Implementation Notes

  • Single Draft File & Epic-Level Slicing Constraints: Ensures only one stories draft (forge-stories-draft.json) and one tasks draft (forge-tasks-draft.json) exists at any time on a ticket by systematically deleting obsolete attachments before uploading newly generated ones. Task drafts are also sliced per Epic and attached (forge-tasks-draft.json containing only the tasks associated with that Epic) to each Epic ticket in addition to attaching the full draft to the Feature ticket, with Epic-level draft attachments automatically deleted upon task approval or provisioning.
  • State Consistency Guard (BR-006): Ensures that any validation or execution error during a draft modification rolls back the draft attachment to the pre-modification version, ensuring no corrupted or partial JSONs are persisted.
  • Truncation Boundary (BR-003): Automatically formats a condensed markdown table instead of verbose task descriptions if the preview exceeds 32,767 characters or contains over 15 items, mitigating Jira API limits.
  • YOLO & Direct Mode Bypass: Detects the forge:yolo label or global yolo_mode to skip the draft review loop and provision issues immediately. It also supports direct ticket creation mode without full YOLO mode via the forge:direct-mode label.

Testing

  • Unit Testing: Thoroughly covered model validation sequences, comment classification regexes, mutation algorithms, custom mock LLM agents, and draft manager behaviors.
  • Integration Testing: Created tests/workflow/test_draft_review_flow.py verifying full end-to-end flows: YOLO bypass, draft attachments, comment comment command modifications, LLM-based draft revisions, truncation rules, and state rollbacks on execution failures. Migrated legacy integration tests in tests/integration/orchestrator/test_workflow_execution.py to the pluggable workflows.
  • Test Locations:
    • tests/unit/models/test_draft.py
    • tests/unit/integrations/jira/test_client_attachments.py
    • tests/unit/workflow/utils/test_draft_manager.py
    • tests/unit/workflow/test_comment_classifier.py
    • tests/workflow/utils/test_comment_command.py
    • tests/unit/integrations/agents/test_agent.py
    • tests/unit/orchestrator/test_worker.py
    • tests/unit/workflow/nodes/test_task_generation.py
    • tests/unit/workflow/nodes/test_direct_mode.py
    • tests/workflow/test_draft_review_flow.py
    • tests/integration/orchestrator/test_workflow_execution.py
    • tests/unit/workflow/nodes/test_docs_updater.py

Related Tickets


Generated by Forge SDLC Orchestrator

@ekuris-redhat ekuris-redhat left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! Please address the following issues found during review:

  1. Missing empty-draft guard in epic decomposition (non-YOLO path)

In src/forge/workflow/nodes/task_generation.py, the non-YOLO path correctly checks if not proposed_tasks_list: and returns a retry state
before touching Jira. The equivalent path in src/forge/workflow/nodes/epic_decomposition.py has no such check. If the agent returns
zero epics in non-YOLO mode, the code creates an empty ForgeDecompositionDraft, uploads it, posts a comment with an empty table, and
pauses the workflow with nothing for the human to approve. Please add the same guard before the draft creation block in decompose_epics:
if epics_data is empty after the LLM call, return a retry state with an appropriate last_error, matching the YOLO path's else branch.

  1. edit_comment skips retry logic

In src/forge/integrations/jira/client.py, the edit_comment method uses client = await self._get_client() and calls client.put(...)
directly. Every other new Jira helper added in this PR (download_attachment, delete_attachment, get_attachments, add_attachment) routes
through _request_with_retry. The PR explicitly advertises rate-limit and retry logic as a feature, but edit_comment will fail
immediately on transient 429 responses. Please refactor it to use _request_with_retry like the other helpers.

  1. Revision comment detection inconsistency

In src/forge/orchestrator/worker.py, the revision comment check uses comment_body.startswith("!"). The classify_comment function in
comment_classifier.py uses _REVISION_PATTERN = re.compile(r"^\s*!") which allows leading whitespace. A comment with a leading space
would be classified as FEEDBACK by the classifier but not caught as a revision comment by the worker, falling through to trigger full
regeneration instead of draft JSON revision. Please change the worker check to use bool(re.match(r"^\s*!", comment_body)) to match the
classifier.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

5 similar comments
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat ekuris-redhat left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, a few things I'd like addressed before we move forward with this one.

First, and most importantly: the .mypy_cache/3.11/ directory got committed — 18 binary cache files are showing up in the diff.
These are local mypy artifacts that should never be tracked. Please remove all of them from the branch and also add
.mypy_cache/ to .gitignore since it's currently missing from there.

Duplicate code in the worker: In worker.py, the block that finds the original review comment and edits it appears twice in a
row — once in the forge command handler and once in the revision comment handler. These are nearly identical. Please extract
that logic into a small helper so we don't have to maintain two copies.

format_review_comment duplication: The stories and tasks branches in draft_manager.py are about 95% identical. The only things
that differ are the header text, the field label, and the approval label. Please collapse them into a single helper that
accepts those as parameters.

Unnecessary LangChain wrapping: In revise_draft_with_feedback, load_prompt already returns a fully formatted string. Wrapping
it in PromptTemplate.from_template("{prompt_text}") just to pass it through is extra ceremony that doesn't add anything. A
direct call to model.ainvoke(prompt_text) would do the same thing with fewer moving parts.

A few smaller things:

The nested from datetime import datetime inside the worker function body should move to the top of the file alongside the
existing from datetime import UTC.

available_repos: Any = set() in epic_decomposition.py should use the concrete type set[str] instead of Any.

In get_attachments, each attachment dict gets both a content and a content_url key pointing to the same value. One is enough.

The _validate_item_params method in DraftManager manually replicates the type checks that Pydantic's DraftItem.model_validate
would already enforce. Consider leaning on Pydantic for this instead.

Finally, provision_epics_from_draft and provision_tasks_from_draft are called from both the worker and the route functions. The
guard on epic_keys prevents double execution, but the split ownership is confusing. Please add a comment explaining why both
call sites need to exist, or consolidate them.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat ekuris-redhat left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few concrete issues I want to flag before merging:

  1. format_review_comment ignores the module's own constants

draft_manager.py defines FORGE_STORIES_DRAFT_FILENAME and FORGE_TASKS_DRAFT_FILENAME at module level, but format_review_comment
hardcodes the literal strings "forge-stories-draft.json" and "forge-tasks-draft.json" in two places. If the filenames ever
change, this method will silently diverge. Replace with the constants.

  1. getattr(settings, "yolo_mode", False) is stale — the field exists

This PR adds yolo_mode: bool = Field(default=False) to config.py, which means the getattr fallback in epic_decomposition.py and
task_generation.py is unnecessary. Use settings.yolo_mode directly.

  1. Duplicate ValidationError import inside _validate_item_params

from pydantic import ValidationError is already imported at the top of draft_manager.py. The same import inside
_validate_item_params's body is redundant — remove it.

  1. YOLO detection copy-pasted 4 times

The same three-component check — "forge:yolo" in labels or getattr(settings, "yolo_mode", False) or state.get("yolo_mode",
False) — appears identically in epic_decomposition.py, task_generation.py, plan_approval.py, and task_approval.py. This should
be a shared helper so all four sites stay in sync.

  1. provision_epics_from_draft and provision_tasks_from_draft use Any for typed parameters

Both functions are declared (state: Any, jira: Any). They should use WorkflowState and JiraClient.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat ekuris-redhat left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the following issues found in the review:

  1. Unguarded split outside the non-blocking try block (update_docs_repo.py:65)

current_repo.split("/", 1) runs before the try block that starts at line 100. If current_repo is empty or missing a /, the
unpacking raises ValueError that is not caught by the non-blocking handler at line 197 — breaking the non-blocking guarantee.
Add a guard before line 65:

if not current_repo or "/" not in current_repo:
logger.warning(f"current_repo is missing or malformed for {ticket_key}, skipping docs repo update")
return state

  1. import subprocess inside function body (update_docs_repo.py:258)

Move the import subprocess statement to the top of the module with the other imports.

  1. Bare except Exception silently swallows errors in _branch_has_commits (update_docs_repo.py:269)

When the git command fails for any reason, the function silently returns False, causing PR creation to be skipped even when the
container committed documentation changes. Add a log statement:

except Exception as e:
logger.warning(f"Could not check for commits in {workspace_path}: {e}")
return False

  1. _branch_has_commits bypasses GitOperations abstraction (update_docs_repo.py:256-270)

All other git operations in this codebase go through GitOperations. Move this logic into a GitOperations method (e.g.,
has_commits_ahead(base_branch: str) -> bool) and call it through docs_git instead of using subprocess directly.

  1. Duplicate default-branch fetch pattern (update_docs_repo.py:69-90)

Lines 69-77 and 82-90 are identical: create a GitHubClient, call get_repository, extract default_branch, log, close. Extract to
a shared helper:

async def _get_repo_default_branch(settings: Settings, owner: str, repo_name: str) -> str:
github = GitHubClient(settings)
try:
data = await github.get_repository(owner, repo_name)
return data.get("default_branch", "main")
except Exception as e:
logger.warning(f"Could not fetch default branch for {owner}/{repo_name}, defaulting to 'main': {e}")
return "main"
finally:
await github.close()

  1. Dead branch in guardrails slice (update_docs_repo.py:160)

guardrails is already defaulted to "" at line 92 via .get("guardrails", ""). The if guardrails else "" branch is unreachable —
""[:2000] and "" are identical. Simplify to guardrails[:2000].

  1. Missing tests

Add tests for:

  • current_repo empty or missing / — verifies the guard added in item 1 returns state without crashing
  • GitError fallback path (lines 121-145) — branch deleted after merge, code falls back to fetching merge commit SHA via
    get_pull_request
  • _create_docs_pr directly — verify fork creation, fork sync, push, PR creation, and Jira comment are all called with the
    correct arguments

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat ekuris-redhat left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues that need addressing:

  1. JSON boundary extraction uses wrong end delimiter (src/forge/integrations/agents/agent.py)

When the LLM response has no markdown code block, the fallback uses max(rfind("}"), rfind("]")) to find the end of the JSON.
This is wrong when the types are mismatched — if the JSON starts with { but there's a trailing ] after the closing } (common in
LLM responses with postamble), max picks the ] and the extracted slice is invalid JSON.

Fix: match the end delimiter to the opening delimiter:

if start_idx == start_brace:
end_idx = cleaned_text.rfind("}")
else:
end_idx = cleaned_text.rfind("]")

  1. Pipe characters in item summary or repo break the Jira markdown table
    (src/forge/workflow/utils/draft_manager.py:format_review_comment)

item.summary and item.repo are interpolated directly into table cells without escaping. A summary like "Support A | B toggle"
produces a broken 4-column row instead of 3. Escape pipe characters before interpolation:

def _escape_cell(text: str) -> str:
return text.replace("|", "\|")

table += f"| {item.id} | {_escape_cell(item.summary)} | {_escape_cell(item.repo or 'unknown')} |\n"

Apply the same escaping in the condensed table path.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@eranco74 eranco74 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great feature — exactly what I had in mind in #218. The draft review loop, /forge commands, state consistency guard, and YOLO bypass are all well thought out. A few issues to look at before merging.

Comment thread src/forge/integrations/agents/agent.py Outdated
return validated_json_str
except json.JSONDecodeError as e:
logger.error(f"Failed to parse LLM response as valid JSON: {e}\nResponse: {response}")
raise ValueError(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security: The raw LLM response is included in this ValueError (Response: {response}). This exception propagates to the worker where it gets posted as a Jira comment via f"Forge command/revision failed: {str(e)}". The LLM response may contain prompt internals or system instructions that shouldn't be visible to users.

Suggestion: log the full response at ERROR level but raise with a sanitized message like "Failed to parse revised draft as valid JSON".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Comment thread src/forge/orchestrator/worker.py Outdated
exc_info=True,
)

error_comment_text = f"❌ Forge command/revision failed: {str(e)}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security: str(e) for HTTP exceptions can contain request URLs, auth headers, or API tokens. Same issue on lines 1780 and 1805 for provisioning errors. Consider sanitizing or using a generic user-facing message while logging the full exception separately (which you're already doing with exc_info=True above).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

try:
epic_keys = await provision_epics_from_draft(state, jira)
# Store the newly created keys
state["epic_keys"] = epic_keys

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: Mutating state["epic_keys"] directly inside a routing function is risky — LangGraph checkpoints state before routing, so these mutations aren't captured. If the process crashes after provision_epics_from_draft creates Jira tickets and deletes the draft but before the next node checkpoints, on restart: epic_keys won't be in the checkpoint, the draft is already deleted, and provisioning would either fail (no draft) or create duplicates.

The worker path (line ~1773) has the same pattern but at least runs outside the LangGraph graph. Consider moving provisioning entirely into the worker or into a dedicated node that checkpoints the created keys before deleting the draft.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

jira = JiraClient()
try:
epic_keys = await provision_epics_from_draft(cast(Any, updated_state), jira)
updated_state["epic_keys"] = epic_keys

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness (minor): Both label-based approval (adding forge:plan-approved) and command-based approval (/forge approve) can trigger provisioning. If both arrive as near-simultaneous webhook events, two workers could each pass the not updated_state.get("epic_keys") guard since each loads the same checkpoint independently. The window is small but could create duplicate tickets. A Jira-side guard (check if children already exist before creating) would make this idempotent.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feedback has already been successfully addressed on the branch via JQL-based Jira-side idempotency guards.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

Forge is addressing PR review feedback now. This status update is informational.

@eranco74

eranco74 commented Aug 2, 2026

Copy link
Copy Markdown

/lgtm

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

/lgtm

thanks. I am now testing it and I will share the results when I have them.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

@@ -0,0 +1,272 @@
"""Post-merge docs repo update node.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems to have been accidentally added to the feature. This is code related to #59

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forge implemented this feedback in the latest pushed revision.

finally:
await jira.close()

elif current_node == "task_approval_gate" and not updated_state.get("task_keys"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current_node == "task_plan_approval_gate" gate needs to be added similarly to how task_approval_gate works. Necessary because task_plan_approval_gate is in _PENDING_APPROVAL_GATES

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Resumption is already fully handled via the is_approved block.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Adding a provisioning block here would cause errors since no drafts exist. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add a provisioning block for task_plan_approval_gate because it is part of the standalone task takeover workflow. Unlike Epic/Feature planning gates, task takeover executes on a single, pre-existing Jira Task and does not decompose into child tickets or have drafts to provision. Resumption is already fully handled via the is_approved block (which clears is_paused and routes to setup_workspace).

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

Detailed description:
- Removed separate docs repository implementation from update_docs_repo.py.
- Deleted corresponding tests TestUpdateDocsRepoRouting and TestCreateDocsPRHelper in test_docs_updater.py.
- Deleted helper method get_project_docs_repo in integrations/jira/client.py.

Closes: AISOS-2294
@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

4 similar comments
@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

/forge rebase

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Rebase triggered by @ekuris-redhat

Merging main into the PR branch and resolving any conflicts. This may take a few minutes.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Merge conflicts resolved and pushed. The PR branch has been updated.

Resolved files: src/forge/orchestrator/worker.py

@ekuris-redhat

Copy link
Copy Markdown
Collaborator Author

/forge rebase

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Rebase triggered by @ekuris-redhat

Merging main into the PR branch and resolving any conflicts. This may take a few minutes.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Merge conflicts resolved and pushed. The PR branch has been updated.

Resolved files: src/forge/workflow/nodes/epic_decomposition.py

@eshulman2

Copy link
Copy Markdown
Collaborator

/forge rebase

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Rebase triggered by @eshulman2

Merging main into the PR branch and resolving any conflicts. This may take a few minutes.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Merge conflicts resolved and pushed. The PR branch has been updated.

Resolved files: src/forge/sandbox/runner.py, tests/unit/workflow/nodes/test_implementation_status_instrumentation.py

@eshulman2 eshulman2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — a few requests before merging:

1. Let users opt into direct ticket creation without full YOLO

Right now the only way to skip the draft/attachment flow for Epic Plan or Task Generation is check_yolo_mode() (forge:yolo label, global YOLO_MODE, or state) — but that also skips the approval gate entirely (route_plan_approval/route_task_plan_approval auto-approve straight through). There's no way to get "direct ticket creation" with a human approval step, which was the pre-this-PR default behavior.

Can we add a label — e.g. forge:direct-mode — that's checked alongside forge:yolo in decompose_epics / generate_tasks, but only switches off the draft/attachment step (skips DraftManager.save_draft_attachment, creates Epics/Tasks directly like the is_yolo branch does today) while still routing to plan_approval_gate / task_approval_gate for approval via forge:plan-approved / forge:task-approved (i.e. not auto-approved like full YOLO)?

That gives users three real choices instead of two:

  • default: draft attachment + review via /forge commands, then approve
  • forge:direct-mode: real sub-tickets created immediately, but still gated on human approval
  • forge:yolo: real sub-tickets created immediately, gate auto-approved (current YOLO)

Please update the ## 🤖 Forge interaction options comment (posted in both decompose_epics and generate_tasks) and the docs (docs/guide/feature-workflow.md, docs/guide/labels.md) to spell out the difference between these three modes, not just draft-vs-yolo.

2. Attach the task draft per-Epic, not only on the Feature

generate_tasks currently only ever writes one aggregate forge-tasks-draft.json on the Feature ticket (FORGE_TASKS_DRAFT_FILENAME, via DraftManager.save_draft_attachment(jira, ticket_key, ...) where ticket_key is the Feature). Epic-level task review isn't possible the way single-epic revision works today for the plan stage (update_single_epic via ! comment on the Epic sub-ticket).

Can we also write a per-Epic slice of the same draft — a forge-tasks-draft.json attachment on each Epic ticket containing only that Epic's proposed tasks — in addition to the full aggregate on the Feature? That would let /forge commands and ! revisions target either:

  • the Feature ticket (all tasks across all epics), or
  • a specific Epic ticket (just that epic's tasks)

mirroring the plan-stage UX. Provisioning (provision_tasks_from_draft) and any comment-command handlers touching the per-epic file will need to stay in sync with the Feature-level one (or the Feature-level draft could be treated as the source of truth and the per-epic ones as read/edit views — happy to discuss which).

3. Confirm attachment cleanup on approval

Looks like this is already handled — provision_epics_from_draft and provision_tasks_from_draft both call DraftManager.delete_draft_attachment after successful provisioning (and also in the idempotency-guard path). Just flagging so it doesn't get lost: if per-Epic attachments are added per #2, they'll need the same delete-after-approval treatment so we don't leave stale drafts on Epic tickets.

@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

1 similar comment
@forgeSmith-bot

Copy link
Copy Markdown
Collaborator

Forge is addressing PR review feedback now. This status update is informational.

Detailed description:
- Support direct ticket creation mode without full YOLO mode via forge:direct-mode label.
- Slice task drafts per Epic and attach forge-tasks-draft.json containing only the tasks associated with that Epic to each Epic ticket, in addition to attaching the full draft to the Feature ticket.
- Automatically delete the Epic-level draft attachments upon task approval/provisioning.
- Updated and added extensive unit tests to verify the correctness of direct mode, draft slicing, and cleanup.

Closes: AISOS-2294-review-fix
…mentation changes

Detailed description:
- Cleaned up, optimized, and sorted imports in task_generation.py and test_direct_mode.py.
- Verified that all unit tests (2057) and integration/flow tests (101) pass successfully.
- Conducted deep code review of direct-mode changes and draft slicing helpers.

Closes: AISOS-2294-review-review-impl
…th model policy

Detailed description:
- Updated 'test_execute_task_changes_successful_workflow' and 'test_build_and_test_recovery_workflow_iterative_self_correction' in 'tests/sandbox/test_task_execution.py' to use 'mock_settings' fixture instead of patching 'get_settings' with a raw 'MagicMock'.
- This fixes a TypeError caused by the recently added model policy system which queries Jira with the settings, and requires a valid Settings object during mock setup.

Closes: AISOS-2294-review-review-impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review ux User experience and interaction improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add decompose draft review step before Jira task creation

5 participants