feat: validate stored plan structure and gate autonomous launches on readiness - #80
Merged
Merged
Conversation
added 2 commits
July 30, 2026 15:29
The plan-write/plan-edit/plan-read tools are now the canonical storage path. Remove the legacy outer-marker fallback prose from the architect prompts and the planner system reminder; the capture code path still handles a wrapped plan silently if one ever appears.
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArchitect planning now persists structured plans in SQL, validates completeness and warnings, restricts autonomous execution tools, and gates loop or group dispatch on readiness. Prompts, documentation, parsing utilities, tests, and version metadata were updated accordingly. ChangesStored Plan Workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Architect
participant PlanWrite
participant StoredPlan
participant Readiness
participant LoopDispatch
Architect->>PlanWrite: author structured plan sections
PlanWrite->>StoredPlan: persist plan in session storage
StoredPlan->>Readiness: inspect structure and warnings
Readiness-->>Architect: return readiness report
Architect->>LoopDispatch: request execution
LoopDispatch->>Readiness: revalidate stored plan
Readiness-->>LoopDispatch: allow or reject launch
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/plan-structure.ts (1)
154-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify why fenced content counts as Objective body content.
objectiveContentfilters by!isFenceBoundaryonly (not!inFence), so genuine text inside a fenced block still satisfies the "non-empty content" check — this mirrorscollectHeadingBodies's deliberate inclusion of non-boundary fenced lines in a heading's body (used forFilespath detection), but the two filters look inconsistent withscanUnfencedLinesat a glance. A short comment here would prevent a future reader from mistaking this for a missed!inFencecheck (as I initially did before cross-checking againstcollectHeadingBodies).✏️ Suggested clarifying comment
const objectiveContent = scanLines(planText).filter(({ index, text, isFenceBoundary }) => index > objectiveHeading.index && index < objectiveBoundaryIndex + // Fenced content still counts as real body content here (only fence+ // boundary lines and structural markers/headings are excluded), matching+ // collectHeadingBodies's treatment of fenced Files content elsewhere. && !isFenceBoundary && text.length > 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 `@src/utils/plan-structure.ts` around lines 154 - 167, Add a concise clarifying comment above the objectiveContent filter explaining that non-boundary lines inside fenced blocks intentionally count as Objective body content, consistent with collectHeadingBodies, while fence boundary lines remain excluded. Do not change the existing !isFenceBoundary filtering or validation behavior.
🤖 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 `@docs/api/variables/VERSION.md`:
- Around line 9-11: Update the generated VERSION API documentation to report
0.7.7 consistently with package.json and src/version.ts, replacing the stale
0.7.6 declaration and value while preserving the existing documentation
structure.
In `@src/agents/architect.ts`:
- Around line 4-14: Update ARCHITECT_TOOL_EXCLUDES to retain the existing
mutation tool names while deriving the plan-related exclusions from the shared
PLAN_AUTHORING_TOOL_NAMES constant. Remove the literal plan, plan_enter, and
plan_exit entries and compose the list using the canonical constant so it stays
aligned with generated permission denies.
---
Nitpick comments:
In `@src/utils/plan-structure.ts`:
- Around line 154-167: Add a concise clarifying comment above the
objectiveContent filter explaining that non-boundary lines inside fenced blocks
intentionally count as Objective body content, consistent with
collectHeadingBodies, while fence boundary lines remain excluded. Do not change
the existing !isFenceBoundary filtering or validation behavior.
🪄 Autofix (Beta)
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: a68fd62d-792b-4424-b31b-667bac6a3223
📒 Files selected for processing (43)
AGENTS.mdREADME.mddocs/agents-and-commands.mddocs/api/README.mddocs/api/_media/agents-and-commands.mddocs/api/_media/architecture.mddocs/api/_media/tools.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/architecture.mddocs/modules.mddocs/tools.mdpackage.jsonsrc/agents/architect-auto.tssrc/agents/architect.tssrc/constants/loop.tssrc/index.tssrc/prompts/agents/architect-auto.mdsrc/prompts/agents/architect.mdsrc/tools/loop.tssrc/tools/plan-authoring.tssrc/utils/architect-auto-output.tssrc/utils/markdown-fences.tssrc/utils/plan-execution.tssrc/utils/plan-structure.tssrc/version.tstest/agents.test.tstest/config.test.tstest/markdown-fences.test.tstest/plan-execution.test.tstest/plan-structure.test.tstest/plugin.test.tstest/services/group-orchestrator.test.tstest/tools/plan-authoring.test.tstest/utils/architect-auto-output.test.ts
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+4
to
+14
| export const ARCHITECT_TOOL_EXCLUDES = [ | ||
| 'apply_patch', | ||
| 'edit', | ||
| 'write', | ||
| 'multiedit', | ||
| 'patch', | ||
| 'task', | ||
| 'plan', | ||
| 'plan_enter', | ||
| 'plan_exit', | ||
| ] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Derive plan-tool exclusions from PLAN_AUTHORING_TOOL_NAMES.
The literal plan, plan_enter, and plan_exit entries can drift from the canonical plan-authoring list and its generated permission denies. Retain the mutation exclusions, but compose the plan-related exclusions from the shared constant.
🤖 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 `@src/agents/architect.ts` around lines 4 - 14, Update ARCHITECT_TOOL_EXCLUDES
to retain the existing mutation tool names while deriving the plan-related
exclusions from the shared PLAN_AUTHORING_TOOL_NAMES constant. Remove the
literal plan, plan_enter, and plan_exit entries and compose the list using the
canonical constant so it stays aligned with generated permission denies.
Source: Coding guidelines
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 freeto 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
Tightens stored-plan validation and rewires the architect prompts and autonomous launch path around a warning-free plan. Every
plan-write/plan-editnow returns actionable structural warnings, and grouped (architect-auto) launches only fire when the stored plan passes readiness inspection.Key changes
Loop Name: short-slugline, per-phase required subsections (Files/Edits/Acceptance Criteria/Verification) with empty detection, trailing Decisions/Conventions/Key Context blocks, absolute-path detection scoped to Files bodies, and section-cap overflow.MAX_LOOP_NAME_LENGTHconstant.inspectArchitectPlanReadinessderived from structure warnings.ARCHITECT_TOOL_EXCLUDES; architect-auto also denies execution/group/loop tools andquestion.PLAN_EXECUTION_LABELS; capture/launch/classifyArchitectFailuregate on stored-plan readiness.PLAN_AUTHORING_TOOL_NAMEScomment updated.Notes
pnpm typecheck && pnpm lint && pnpm test && pnpm buildall green (2807 tests passing).DR-a10b0693-001(remote goal/plan launch acknowledgement) belongs to PR feat: add goal-brief launch flow with server-owned prompt and goal-authoring tools #79 (feature/goal-brief-launch) and is intentionally left untouched here;DR-a10b0693-002is already closed. Norefs/notes/pr-reviewfindings are attached to this branch's commits.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores