Fix agent output summary for pipeline steps#812
Merged
gsxdsm merged 4 commits intoAutoMaker-Org:v1.0.0rcfrom Feb 26, 2026
Merged
Fix agent output summary for pipeline steps#812gsxdsm merged 4 commits intoAutoMaker-Org:v1.0.0rcfrom
gsxdsm merged 4 commits intoAutoMaker-Org:v1.0.0rcfrom
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes pipeline step summary extraction and accumulation: agents produce mandated blocks, executor extracts and saves per-step summaries (status-aware), FeatureStateManager accumulates them into feature.summary while preserving pipeline statuses, and the UI displays multi-phase summaries with navigation and agent-output view.
|
| Cohort / File(s) | Summary |
|---|---|
Repository config /.geminiignore |
Add autogenerated ignore file listing common paths to exclude from Gemini CLI discovery. |
Server startup apps/server/src/index.ts |
Always attempts to resume interrupted features on startup (removed outer conditional gating). |
Agent execution & task parsing apps/server/src/services/agent-executor.ts, apps/server/src/services/agent-executor-types.ts, apps/server/src/services/spec-parser.ts |
Add status to AgentExecutionOptions; centralize extractAndSaveSessionSummary; strip scaffold helper; detectTaskCompleteMarker now returns {id, summary?}; task completion uses marker.summary. |
Feature state & pipeline handling apps/server/src/services/feature-state-manager.ts, apps/server/src/services/pipeline-orchestrator.ts, apps/server/src/services/execution-service.ts |
Use isPipelineStatus guard; preserve pipeline_* statuses during resets; accumulate per-step summaries with deterministic headers/separators; updateTaskStatus accepts optional summary; orchestrator passes pipeline_<step.id> status and appends CRITICAL <summary> directive to prompts. |
Auto-mode & eligibility apps/server/src/services/auto-mode/facade.ts |
Add isFeatureEligibleForAutoMode(...); thread optional status through RunAgentFn into agent execution. |
Types & constants libs/types/src/pipeline.ts, libs/types/src/index.ts, libs/types/src/feature.ts |
Add isPipelineStatus type guard and PIPELINE_SUMMARY_* constants; re-export isPipelineStatus; add optional ParsedTask.summary?: string. |
Prompts libs/prompts/src/defaults.ts |
Pipeline step prompt now requires a CRITICAL <summary> block in a strict format after each step. |
UI parsing & selection utilities apps/ui/src/lib/log-parser.ts, apps/ui/src/lib/summary-selection.ts |
Add multi-phase parsing APIs (parsePhaseSummaries, parseAllPhaseSummaries, extractPhaseSummary, extractImplementationSummary, isAccumulatedSummary) and getFirstNonEmptySummary for summary prioritization. |
UI components apps/ui/src/components/views/board-view/.../agent-info-panel.tsx, .../summary-dialog.tsx, .../agent-output-modal.tsx, .../completed-features-modal.tsx |
Add realtime per-task summaries, compute effectiveSummary preferring server-accumulated data, multi-phase SummaryDialog/AgentOutputModal with PhaseEntryCard and StepNavigator, agent-output view; SummaryDialogProps gains optional projectPath. |
Tests — server & UI apps/server/tests/unit/..., apps/server/tests/unit/ui/... |
Large additions and updates: spec-parser, agent-executor summary extraction, pipeline summary accumulation, auto-mode eligibility, execution-service, many UI parsing/priority and integration tests covering multi-step accumulation and display. |
sequenceDiagram
participant Agent as Agent Executor
participant FSM as FeatureStateManager
participant Store as Storage (FS/DB)
participant Bus as EventBus
participant UI as UI Client
Agent->>Agent: run pipeline step (opts.status = "pipeline_<id>")
Agent->>FSM: extractAndSaveSessionSummary(projectPath, featureId, newContent, prevContent, callbacks, status)
Note over FSM: parse <summary> block or detect [TASK_COMPLETE] with optional summary
FSM->>FSM: accumulate per-step header into feature.summary
FSM->>Store: atomicWriteJson(feature)
Note over Store: persist BEFORE emit
FSM->>Bus: emit auto_mode_summary (featureId, accumulated summary)
Bus->>UI: deliver auto_mode_summary
UI->>UI: invalidate cache, fetch updated feature
UI->>UI: parseAllPhaseSummaries() → render PhaseEntryCard / StepNavigator
Bug🐇 I nibbled logs through midnight light,
I stitched each step into headers bright,
From Implementation to Review and Test,
I file each summary, tidy and pressed,
A rabbit's ledger—pipeline done right.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 79.49% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Fix agent output summary for pipeline steps' directly describes the main change in the changeset. It identifies the specific feature being fixed (agent output summary) and clarifies the context (pipeline steps). |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Warning
Review ran into problems
🔥 Problems
Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Contributor
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the handling of agent output summaries, particularly within multi-step pipelines. The core objective is to ensure that the comprehensive narrative of a feature's development, from initial implementation through testing and review, is accurately captured and presented. This is achieved by introducing a robust mechanism for accumulating summaries across pipeline stages, standardizing the format of agent-generated summaries, and enhancing the UI to intelligently parse and display these multi-phase summaries. These changes provide users with a more detailed and chronologically ordered view of a feature's progress, improving transparency and understanding of the automated development process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a substantial and well-executed pull request that adds robust support for capturing and displaying summaries from multi-step pipeline executions. The backend changes in FeatureStateManager correctly accumulate summaries, and the AgentExecutor is cleanly refactored to extract summaries only from new content. The frontend has been significantly enhanced with a new step navigator in the summary dialogs, providing a much-improved user experience for pipeline features. The inclusion of a comprehensive suite of new tests is particularly commendable and greatly increases confidence in these complex changes. I have one minor suggestion to improve efficiency by avoiding an unnecessary file read in the AgentExecutor.
Comment on lines
+386
to
+398
| try { | ||
| const featureDir = getFeatureDir(projectPath, featureId); | ||
| const featurePath = path.join(featureDir, 'feature.json'); | ||
| const feature = (await secureFs.readFile(featurePath)).toString(); | ||
| const featureData = JSON.parse(feature); | ||
| if (isPipelineStatus(featureData.status)) { | ||
| logger.warn( | ||
| `[AgentExecutor] Mandatory summary extraction failed for pipeline feature ${featureId} (status="${featureData.status}")` | ||
| ); | ||
| } | ||
| } catch { | ||
| /* ignore failures in warning logging */ | ||
| } |
Contributor
There was a problem hiding this comment.
This try-catch block performs a file I/O operation (readFile) just to check the feature's status for the purpose of logging a warning. This is a bit inefficient, as the caller (ExecutionService) already has the full feature object and could pass the status down.
To improve this, consider adding an optional status?: string to the AgentExecutionOptions type. The ExecutionService can then populate this from the feature object it has already loaded.
This would allow you to simplify this block to something like:
// In extractAndSaveSessionSummary method...
} else {
// If we're in a pipeline step, a summary is expected. Log a warning if it's missing.
if (isPipelineStatus(options.status)) { // Assuming status is passed via options
logger.warn(
`[AgentExecutor] Mandatory summary extraction failed for pipeline feature ${featureId} (status="${options.status}")`
);
}
}This change would avoid the extra file read and make the check more efficient and less prone to I/O errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
apps/server/src/services/pipeline-orchestrator.ts (1)
166-180: Consider using the prompt template instead of hardcoding.The summary directive is duplicated between this hardcoded implementation and
DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATEinlibs/prompts/src/defaults.ts. If the prompt customization system is meant to be used here, this hardcoded string could drift from the template.That said, if this method intentionally bypasses template customization for consistency, this approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/pipeline-orchestrator.ts` around lines 166 - 180, The hardcoded summary/prompt block in the return of the method (which concatenates prompt + step.instructions + the summary directive) duplicates the directive defined in DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATE; replace the inline string with usage of that template instead—import DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATE from libs/prompts/src/defaults.ts and interpolate step.instructions and step.name (and any other variables) into the template rather than embedding the summary text here, or if this method must bypass templates, add a clear comment explaining why and keep the strings consistent by referencing the template constant.apps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsx (1)
1-1: Consider removing@ts-nocheckdirective.Suppressing all TypeScript checks makes it harder to catch type errors during development. If there are specific type issues, consider using targeted
@ts-expect-erroror//@ts-ignore`` comments with explanations, or fix the underlying type issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsx` at line 1, Remove the file-level "@ts-nocheck" and instead fix the underlying type issues in this module: add proper typings to the CompletedFeaturesModal component props and state, and annotate helper functions like filterCompletedFeatures and groupFeaturesByStatus (or any similarly named filtering/grouping utilities) so TypeScript can validate them; if there are unavoidable single-line type exceptions, replace the blanket directive with precise "// `@ts-expect-error`" or "// `@ts-ignore`" comments with brief explanations immediately above the offending lines. Ensure exported types/interfaces used by the component (e.g., Feature, FeatureStatus) are imported or declared and update function signatures to return typed values so the file can be type-checked without the global suppression.libs/types/src/pipeline.ts (1)
22-27: Consolidate duplicateisPipelineStatusimplementations to centralized type guard.The centralized implementation in
libs/typesis properly defined as a type guard with null/undefined handling. However, duplicate implementations still exist and should be replaced with imports from@automaker/types:
apps/ui/src/components/views/board-view/constants.ts(line 142) — local function that returns boolean, used by status-badge.tsxapps/server/src/services/pipeline-service.ts(line 328) — service method that lacks null/undefined handling, used by pipeline-orchestrator.tsSeveral services already import the centralized version (feature-state-manager.ts, agent-executor.ts, auto-mode/facade.ts), demonstrating it's the intended standard. Consolidating these duplicates will improve type safety and reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/pipeline.ts` around lines 22 - 27, Duplicate local implementations of the isPipelineStatus check should be removed and replaced with the centralized type guard exported from `@automaker/types` (the existing function name is isPipelineStatus); update the files that currently define local versions (the boolean-returning helper in board-view constants and the service method in pipeline-service) to import isPipelineStatus from `@automaker/types`, ensure callers rely on its type-guard behavior (handle string | null | undefined where needed) and remove the local functions so all code uses the single source of truth.apps/server/tests/unit/ui/summary-auto-scroll.test.ts (1)
28-36: Avoid mirroring production logic directly in the test.This helper copy can drift from
agent-output-modalbehavior over time. Prefer extractingisScrollAtBottominto a shared utility and importing it in both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/ui/summary-auto-scroll.test.ts` around lines 28 - 36, The test contains a duplicated implementation of isScrollAtBottom which can drift from the production implementation in agent-output-modal; extract the logic into a shared utility (e.g., export a single isScrollAtBottom function) used by the agent-output-modal component and import that same utility into apps/server/tests/unit/ui/summary-auto-scroll.test.ts so the test calls the shared isScrollAtBottom instead of its local copy, ensuring one canonical implementation shared between component and test.apps/server/src/services/feature-state-manager.ts (1)
597-610: Consider simplifying the regex to mitigate ReDoS concerns.The static analysis tool flagged this dynamically constructed regex for potential ReDoS. While
escapedStepHeaderis properly escaped, the pattern[\s\S]*?combined with the lookahead(?=...)could exhibit polynomial backtracking on malformed input.Since
feature.summaryis typically small and controlled, the practical risk is low. However, for defense-in-depth, consider usingindexOforsplitinstead of regex for section replacement:♻️ Alternative approach using string operations
- const escapedStepHeader = stepHeader.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - // The separator is actual newlines: \n\n---\n\n - // We need to escape it for regex: match literal \n as \\n in the pattern - const escapedSeparator = '\\n\\n---\\n\\n'; - - // Regex finds the header and everything until the next separator or end of string - const stepRegex = new RegExp( - `(${escapedStepHeader}\\n\\n[\\s\\S]*?)(?=${escapedSeparator}|$)`, - 'g' - ); - - if (stepRegex.test(feature.summary)) { - // Replace the existing section - feature.summary = feature.summary.replace(stepRegex, stepSection); + const headerIndex = feature.summary.indexOf(stepHeader + '\n\n'); + if (headerIndex !== -1) { + // Find the end of this section (next separator or end of string) + const separator = '\n\n---\n\n'; + const sectionStart = headerIndex; + const afterHeader = headerIndex + stepHeader.length + 2; // skip header + \n\n + const nextSeparator = feature.summary.indexOf(separator, afterHeader); + const sectionEnd = nextSeparator !== -1 ? nextSeparator : feature.summary.length; + // Replace the existing section + feature.summary = + feature.summary.substring(0, sectionStart) + + stepSection + + feature.summary.substring(sectionEnd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 597 - 610, The dynamic regex built in stepRegex (using escapedStepHeader, escapedSeparator and [\\s\\S]*?) can cause ReDoS; replace the regex-based extraction/replace with safe string operations: find the start index via feature.summary.indexOf(escapedStepHeader), then find the next separator index via feature.summary.indexOf('\\n\\n---\\n\\n', startIdx) (or -1 for end), and replace the substring between startIdx and separatorIdx with stepSection (or append if header not found); update the code paths that currently call stepRegex.test(...) and feature.summary.replace(stepRegex, stepSection) to use this index/slice + concatenation approach to avoid backtracking in stepRegex.apps/server/tests/unit/ui/log-parser-phase-summary.test.ts (1)
16-44: Risk of implementation drift between test and production code.The test file reimplements
parsePhaseSummariesand related functions locally, but comparing to the actual UI implementation inapps/ui/src/lib/log-parser.ts(from the relevant code snippets), there are differences:
- The UI version uses a
getPhaseSectionshelper that handlesleadingImplementationSectionfor backward compatibility with mixed formats- The local test implementation (line 30-41) splits on separator and looks for headers, but doesn't handle headerless leading content
This means tests may pass here but the actual UI could behave differently. Consider either:
- Importing from the UI file (as done in
log-parser-mixed-format.test.ts)- Adding a comment acknowledging this is testing a simplified version
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/tests/unit/ui/log-parser-phase-summary.test.ts` around lines 16 - 44, The test reimplements parsePhaseSummaries but omits the UI's getPhaseSections/leadingImplementationSection behavior, causing drift; either import the real parsePhaseSummaries/getPhaseSections from the UI module (as done in log-parser-mixed-format.test.ts) or update this test helper to match the UI: add handling for a headerless leading section (leadingImplementationSection) before splitting on the "\n\n---\n\n" separator and preserve backward-compatible section parsing, ensuring the function name parsePhaseSummaries and the helper getPhaseSections/leadingImplementationSection are referenced and kept in sync with the UI implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/spec-parser.ts`:
- Around line 108-115: The summary capture is prematurely stopping at any '['
because the regex uses ([^\n\[]+); update the regex in spec-parser.ts (the match
variable building the result) to allow '[' inside summaries and only stop at a
newline or the next marker. Replace the current pattern
/\[TASK_COMPLETE\]\s*(T\d{3})(?::\s*([^\n\[]+))?/i with one that captures up to
a newline or next marker, e.g. use ([^\n]+?) with a lookahead such as
(?=\n|\[TASK_[A-Z_]+\]|$) so the returned id (match[1]) and summary (match[2])
include text like "array[index]" intact.
In `@apps/server/tests/unit/types/pipeline-types.test.ts`:
- Around line 12-16: The PipelineStatus type and its test allow the invalid
value "pipeline_" (empty suffix); update the type and tests so pipeline statuses
require a non-empty suffix. Change the PipelineStatus literal type (currently
'pipeline_${string}') to require at least one character after the underscore
(e.g., use a branded/type constraint or narrow pattern to disallow empty
suffixes) and update isPipelineStatus to check the prefix plus non-empty
remainder (refer to isPipelineStatus) and adjust the unit test in
pipeline-types.test.ts to remove or change the
expect(isPipelineStatus('pipeline_')).toBe(true) case to assert false or provide
a valid suffix. Ensure downstream usages that call status.replace('pipeline_',
'') continue to work with non-empty suffixes.
In `@apps/server/tests/unit/ui/agent-output-summary-e2e.test.ts`:
- Around line 67-107: The test helper duplicates production summary-parsing
behavior (parsePhaseSummaries, extractSummary, isAccumulatedSummary) but the
tests still use featureSummary || extractSummary(rawOutput) which differs from
production's getFirstNonEmptySummary and can mis-handle whitespace-only
summaries; replace the test's selection logic to call the real production helper
getFirstNonEmptySummary(...) (import it from the production module) instead of
falling back to extractSummary, and ensure parsePhaseSummaries and
isAccumulatedSummary remain consistent with production behavior or are removed
in favor of the shared production helpers referenced by name so
whitespace/server-summary edge cases match production; apply the same change
where similar logic appears (the other occurrences mentioned).
In `@apps/server/tests/unit/ui/phase-summary-parser.test.ts`:
- Around line 16-101: The test file currently reimplements production parsing
helpers (parsePhaseSummaries, extractPhaseSummary, extractImplementationSummary,
isAccumulatedSummary) which causes the suite to remain green even if
apps/ui/src/lib/log-parser.ts regresses; replace the local duplicates by
importing those functions from apps/ui/src/lib/log-parser.ts (or, if import is
impossible, add a clear TODO comment noting these are mirrored and add a single
integration test that asserts parity with the real implementations) so tests
validate the real production logic rather than a duplicated copy.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx`:
- Line 237: The current assignment "summary: realtimeSummary || task.summary"
treats empty strings or explicit resets as falsy and keeps old summaries; change
it to use a nullish or explicit-undefined check so empty/reset values win —
e.g., set summary using "realtimeSummary ?? task.summary" or "realtimeSummary
!== undefined ? realtimeSummary : task.summary" for the field where
'realtimeSummary' and 'task.summary' are used, and apply the same change to the
other occurrence around the block that spans the lines referenced (the second
place using realtimeSummary/task.summary at 296-301).
In
`@apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx`:
- Around line 166-180: The scroll effect may not run when rawSummary changes if
activePhaseIndex is already 0, so update the scroll useEffect (the effect that
reads contentRef, phaseCards, activePhaseIndex, hasMultiplePhases) to also
depend on rawSummary and to handle edge cases: when rawSummary changes,
recompute phaseCards from contentRef and pick a safe targetCard (fallback to
index 0 if activePhaseIndex is out of range) then call scrollIntoView; keep
setActivePhaseIndex usage as-is but ensure the effect triggers on rawSummary so
the new first phase is scrolled even when activePhaseIndex remains 0.
- Around line 56-76: The root div that currently uses onClick for PhaseEntryCard
is not keyboard accessible; update the root container (the div with the onClick
prop and className) to add role="button", tabIndex={0}, and an onKeyDown handler
that listens for Enter and Space and invokes the existing onClick (calling
preventDefault for Space) so keyboard users can activate the card; ensure the
onKeyDown references the same onClick prop so behavior stays consistent with
mouse clicks.
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`:
- Around line 236-242: The auto-scroll indicator reads summaryAutoScrollRef
during render but handleSummaryScroll only mutates the ref so the component
never re-renders; change summaryAutoScrollRef to React state (e.g., const
[summaryAutoScroll, setSummaryAutoScroll] = useState<boolean>(true)) and in
handleSummaryScroll compute isAtBottom and call setSummaryAutoScroll(isAtBottom)
instead of mutating summaryAutoScrollRef.current; update the render/indicator
(where it currently reads summaryAutoScrollRef) to read the new state variable;
keep summaryScrollRef as a ref for DOM measurements but use state for UI-driven
values so the indicator updates reliably.
---
Nitpick comments:
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 597-610: The dynamic regex built in stepRegex (using
escapedStepHeader, escapedSeparator and [\\s\\S]*?) can cause ReDoS; replace the
regex-based extraction/replace with safe string operations: find the start index
via feature.summary.indexOf(escapedStepHeader), then find the next separator
index via feature.summary.indexOf('\\n\\n---\\n\\n', startIdx) (or -1 for end),
and replace the substring between startIdx and separatorIdx with stepSection (or
append if header not found); update the code paths that currently call
stepRegex.test(...) and feature.summary.replace(stepRegex, stepSection) to use
this index/slice + concatenation approach to avoid backtracking in stepRegex.
In `@apps/server/src/services/pipeline-orchestrator.ts`:
- Around line 166-180: The hardcoded summary/prompt block in the return of the
method (which concatenates prompt + step.instructions + the summary directive)
duplicates the directive defined in
DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATE; replace the inline string with
usage of that template instead—import
DEFAULT_AUTO_MODE_PIPELINE_STEP_PROMPT_TEMPLATE from
libs/prompts/src/defaults.ts and interpolate step.instructions and step.name
(and any other variables) into the template rather than embedding the summary
text here, or if this method must bypass templates, add a clear comment
explaining why and keep the strings consistent by referencing the template
constant.
In `@apps/server/tests/unit/ui/log-parser-phase-summary.test.ts`:
- Around line 16-44: The test reimplements parsePhaseSummaries but omits the
UI's getPhaseSections/leadingImplementationSection behavior, causing drift;
either import the real parsePhaseSummaries/getPhaseSections from the UI module
(as done in log-parser-mixed-format.test.ts) or update this test helper to match
the UI: add handling for a headerless leading section
(leadingImplementationSection) before splitting on the "\n\n---\n\n" separator
and preserve backward-compatible section parsing, ensuring the function name
parsePhaseSummaries and the helper getPhaseSections/leadingImplementationSection
are referenced and kept in sync with the UI implementation.
In `@apps/server/tests/unit/ui/summary-auto-scroll.test.ts`:
- Around line 28-36: The test contains a duplicated implementation of
isScrollAtBottom which can drift from the production implementation in
agent-output-modal; extract the logic into a shared utility (e.g., export a
single isScrollAtBottom function) used by the agent-output-modal component and
import that same utility into
apps/server/tests/unit/ui/summary-auto-scroll.test.ts so the test calls the
shared isScrollAtBottom instead of its local copy, ensuring one canonical
implementation shared between component and test.
In
`@apps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsx`:
- Line 1: Remove the file-level "@ts-nocheck" and instead fix the underlying
type issues in this module: add proper typings to the CompletedFeaturesModal
component props and state, and annotate helper functions like
filterCompletedFeatures and groupFeaturesByStatus (or any similarly named
filtering/grouping utilities) so TypeScript can validate them; if there are
unavoidable single-line type exceptions, replace the blanket directive with
precise "// `@ts-expect-error`" or "// `@ts-ignore`" comments with brief
explanations immediately above the offending lines. Ensure exported
types/interfaces used by the component (e.g., Feature, FeatureStatus) are
imported or declared and update function signatures to return typed values so
the file can be type-checked without the global suppression.
In `@libs/types/src/pipeline.ts`:
- Around line 22-27: Duplicate local implementations of the isPipelineStatus
check should be removed and replaced with the centralized type guard exported
from `@automaker/types` (the existing function name is isPipelineStatus); update
the files that currently define local versions (the boolean-returning helper in
board-view constants and the service method in pipeline-service) to import
isPipelineStatus from `@automaker/types`, ensure callers rely on its type-guard
behavior (handle string | null | undefined where needed) and remove the local
functions so all code uses the single source of truth.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.geminiignoreapps/server/src/index.tsapps/server/src/services/agent-executor.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/execution-service.tsapps/server/src/services/feature-state-manager.tsapps/server/src/services/pipeline-orchestrator.tsapps/server/src/services/spec-parser.tsapps/server/tests/unit/services/agent-executor-summary.test.tsapps/server/tests/unit/services/auto-mode-facade.test.tsapps/server/tests/unit/services/execution-service.test.tsapps/server/tests/unit/services/feature-state-manager.test.tsapps/server/tests/unit/services/pipeline-orchestrator-prompts.test.tsapps/server/tests/unit/services/pipeline-summary-accumulation.test.tsapps/server/tests/unit/services/spec-parser.test.tsapps/server/tests/unit/types/pipeline-types.test.tsapps/server/tests/unit/ui/agent-output-summary-e2e.test.tsapps/server/tests/unit/ui/agent-output-summary-priority.test.tsapps/server/tests/unit/ui/log-parser-mixed-format.test.tsapps/server/tests/unit/ui/log-parser-phase-summary.test.tsapps/server/tests/unit/ui/log-parser-summary.test.tsapps/server/tests/unit/ui/phase-summary-parser.test.tsapps/server/tests/unit/ui/summary-auto-scroll.test.tsapps/server/tests/unit/ui/summary-source-flow.integration.test.tsapps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsxapps/ui/src/lib/log-parser.tsapps/ui/src/lib/summary-selection.tslibs/prompts/src/defaults.tslibs/types/src/feature.tslibs/types/src/index.tslibs/types/src/pipeline.ts
Comment on lines
+12
to
+16
| it('should return true for pipeline_ prefix with any suffix', () => { | ||
| expect(isPipelineStatus('pipeline_')).toBe(true); | ||
| expect(isPipelineStatus('pipeline_123')).toBe(true); | ||
| expect(isPipelineStatus('pipeline_step_abc_123')).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find isPipelineStatus implementation
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "isPipelineStatus" | grep -v test | grep -v node_modulesRepository: AutoMaker-Org/automaker
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Also search for the implementation directly using ast-grep
ast-grep --pattern 'function isPipelineStatus($$$) { $$$ }' || ast-grep --pattern 'export const isPipelineStatus = $_'Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check actual usage of 'pipeline_' with empty suffix in codebase
rg "pipeline_['\"]" -t ts --type tsx -g '!*.test.ts' -g '!*.spec.ts' | head -20Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read the isPipelineStatus implementation
cat -n ./libs/types/src/pipeline.tsRepository: AutoMaker-Org/automaker
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Read the test file to see all test cases
cat -n ./apps/server/tests/unit/types/pipeline-types.test.tsRepository: AutoMaker-Org/automaker
Length of output: 2358
🏁 Script executed:
#!/bin/bash
# Search for actual 'pipeline_' usage (empty suffix) in non-test files
rg "pipeline_" -t ts -g '!*.test.ts' -g '!*.spec.ts' | grep -E "pipeline_['\"]|\['pipeline_" | head -20Repository: AutoMaker-Org/automaker
Length of output: 2406
Consider restricting PipelineStatus to require a non-empty suffix.
The test at line 13 is correct per the current implementation: isPipelineStatus('pipeline_') returns true because startsWith('pipeline_') matches. However, the codebase consistently processes pipeline statuses by extracting the suffix after pipeline_ (e.g., status.replace('pipeline_', '') in pipeline-service.ts, feature-state-manager.ts, and UI constants), expecting a valid step ID. A bare 'pipeline_' would result in an empty suffix, which is likely invalid downstream. The PipelineStatus type definition ('pipeline_${string}') is too permissive. Consider updating it to enforce a non-empty suffix, or document that 'pipeline_' without a step ID is an invalid state that should never occur in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/types/pipeline-types.test.ts` around lines 12 - 16,
The PipelineStatus type and its test allow the invalid value "pipeline_" (empty
suffix); update the type and tests so pipeline statuses require a non-empty
suffix. Change the PipelineStatus literal type (currently 'pipeline_${string}')
to require at least one character after the underscore (e.g., use a branded/type
constraint or narrow pattern to disallow empty suffixes) and update
isPipelineStatus to check the prefix plus non-empty remainder (refer to
isPipelineStatus) and adjust the unit test in pipeline-types.test.ts to remove
or change the expect(isPipelineStatus('pipeline_')).toBe(true) case to assert
false or provide a valid suffix. Ensure downstream usages that call
status.replace('pipeline_', '') continue to work with non-empty suffixes.
Comment on lines
+16
to
+101
| // Mirror the phase summary parsing functions from apps/ui/src/lib/log-parser.ts | ||
| // We can't import directly because it's a UI file | ||
|
|
||
| /** | ||
| * Parses an accumulated summary string into individual phase summaries. | ||
| */ | ||
| function parsePhaseSummaries(summary: string | undefined): Map<string, string> { | ||
| const phaseSummaries = new Map<string, string>(); | ||
|
|
||
| if (!summary || !summary.trim()) { | ||
| return phaseSummaries; | ||
| } | ||
|
|
||
| // Split by the horizontal rule separator | ||
| const sections = summary.split(/\n\n---\n\n/); | ||
|
|
||
| for (const section of sections) { | ||
| // Match the phase header pattern: ### Phase Name | ||
| const headerMatch = section.match(/^###\s+(.+?)(?:\n|$)/); | ||
| if (headerMatch) { | ||
| const phaseName = headerMatch[1].trim().toLowerCase(); | ||
| // Extract content after the header (skip the header line and leading newlines) | ||
| const content = section.substring(headerMatch[0].length).trim(); | ||
| phaseSummaries.set(phaseName, content); | ||
| } | ||
| } | ||
|
|
||
| return phaseSummaries; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts a specific phase summary from an accumulated summary string. | ||
| */ | ||
| function extractPhaseSummary(summary: string | undefined, phaseName: string): string | null { | ||
| const phaseSummaries = parsePhaseSummaries(summary); | ||
| const normalizedPhaseName = phaseName.toLowerCase(); | ||
| return phaseSummaries.get(normalizedPhaseName) || null; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the implementation phase summary from an accumulated summary string. | ||
| */ | ||
| function extractImplementationSummary(summary: string | undefined): string | null { | ||
| if (!summary || !summary.trim()) { | ||
| return null; | ||
| } | ||
|
|
||
| const phaseSummaries = parsePhaseSummaries(summary); | ||
|
|
||
| // Try exact match first | ||
| const implementationContent = phaseSummaries.get('implementation'); | ||
| if (implementationContent) { | ||
| return implementationContent; | ||
| } | ||
|
|
||
| // Fallback: find any phase containing "implement" | ||
| for (const [phaseName, content] of phaseSummaries) { | ||
| if (phaseName.includes('implement')) { | ||
| return content; | ||
| } | ||
| } | ||
|
|
||
| // If no phase summaries found, the summary might not be in accumulated format | ||
| // (legacy or non-pipeline feature). In this case, return the whole summary | ||
| // if it looks like a single summary (no phase headers). | ||
| if (!summary.includes('### ') && !summary.includes('\n---\n')) { | ||
| return summary; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a summary string is in the accumulated multi-phase format. | ||
| */ | ||
| function isAccumulatedSummary(summary: string | undefined): boolean { | ||
| if (!summary || !summary.trim()) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check for the presence of phase headers with separator | ||
| const hasMultiplePhases = | ||
| summary.includes('\n\n---\n\n') && summary.match(/###\s+.+/g)?.length > 0; | ||
|
|
||
| return hasMultiplePhases; | ||
| } |
Contributor
There was a problem hiding this comment.
These tests are validating mirrored logic, not production logic.
Lines 16-101 reimplement parser helpers locally. That allows this suite to stay green even when apps/ui/src/lib/log-parser.ts regresses or changes behavior.
💡 Suggested refactor
-import { describe, it, expect } from 'vitest';
-
-// Mirror the phase summary parsing functions from apps/ui/src/lib/log-parser.ts
-// We can't import directly because it's a UI file
+import { describe, it, expect } from 'vitest';
+import {
+ parsePhaseSummaries,
+ extractPhaseSummary,
+ extractImplementationSummary,
+ isAccumulatedSummary,
+} from '../../../../ui/src/lib/log-parser.ts';
-
-// ...remove local helper implementations...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/tests/unit/ui/phase-summary-parser.test.ts` around lines 16 -
101, The test file currently reimplements production parsing helpers
(parsePhaseSummaries, extractPhaseSummary, extractImplementationSummary,
isAccumulatedSummary) which causes the suite to remain green even if
apps/ui/src/lib/log-parser.ts regresses; replace the local duplicates by
importing those functions from apps/ui/src/lib/log-parser.ts (or, if import is
impossible, add a clear TODO comment noting these are mirrored and add a single
integration test that asserts parity with the real implementations) so tests
validate the real production logic rather than a duplicated copy.
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx
Outdated
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx
Show resolved
Hide resolved
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (1)
72-72:⚠️ Potential issue | 🟡 MinorExplicit summary resets are still dropped, so stale task summaries can remain.
Line 300-Line 304 converts empty/reset summaries to
null, and Line 237 then falls back via??totask.summary. That means a real reset event can still show stale summary text.Proposed fix
- const realtimeSummary = taskSummaryMap.get(task.id); + const hasRealtimeSummary = taskSummaryMap.has(task.id); + const realtimeSummary = taskSummaryMap.get(task.id); ... - summary: realtimeSummary ?? task.summary, + summary: hasRealtimeSummary ? realtimeSummary : task.summary, ... - // Allow empty string (reset) or non-empty string to be set - const summary = - typeof event.summary === 'string' && event.summary.trim().length > 0 - ? event.summary - : null; - newMap.set(taskEvent.taskId, summary); + // Preserve explicit empty-string resets from events. + const nextSummary = typeof event.summary === 'string' ? event.summary : ''; + newMap.set(taskEvent.taskId, nextSummary);#!/bin/bash set -euo pipefail FILE="$(fd 'agent-info-panel.tsx' | head -n1)" echo "Inspecting: $FILE" echo "== Summary reset/fallback path ==" rg -n -C2 'taskSummaryMap|realtimeSummary|summary:\s*realtimeSummary|event.summary|newMap.set\(taskEvent.taskId' "$FILE" echo "== TypeScript nullability config ==" fd 'tsconfig*.json' | while read -r cfg; do echo "--- $cfg ---" rg -n '"strict"\s*:|"strictNullChecks"\s*:' "$cfg" || true doneExpected verification:
- You should see empty summaries being coerced before writing to the map.
- If
strictNullChecksis enabled, writingnullintoMap<string, string>is invalid.Also applies to: 214-215, 237-237, 296-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx` at line 72, The task summary reset logic is turning empty/reset summaries into null which then falls back to stale task.summary via the ?? operator; update the handling in the taskSummaryMap update (where newMap.set(taskEvent.taskId, ...), realtimeSummary, and event.summary are processed) to store an explicit empty string or remove the map entry on reset instead of writing null, and adjust any consumers that use taskSummaryMap (including the ?? fallback at the render site) to treat empty string as an intentional reset (or check map.has(taskId) before falling back to task.summary) so resets no longer display stale text; modify setTaskSummaryMap and the code that computes the stored value accordingly.
🧹 Nitpick comments (1)
apps/server/src/services/agent-executor-types.ts (1)
47-48: Narrowstatusto shared status type instead of plainstring.Line 48 weakens type safety for the new summary path. Prefer
FeatureStatusWithPipeline(orFeature['status']) so invalid statuses are caught at compile time.♻️ Suggested diff
import type { PlanningMode, ThinkingLevel, ReasoningEffort, ParsedTask, ClaudeCompatibleProvider, Credentials, + FeatureStatusWithPipeline, } from '@automaker/types'; @@ - status?: string; + status?: FeatureStatusWithPipeline;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor-types.ts` around lines 47 - 48, The optional property `status` currently typed as `string` should be narrowed to the shared feature status type; replace `status?: string` with `status?: FeatureStatusWithPipeline` (or `Feature['status']`) to enforce valid statuses at compile time and import the `FeatureStatusWithPipeline` type (or `Feature`) from its module; update the declaration where `status` is defined so downstream code uses the stricter type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/agent-executor.ts`:
- Around line 388-394: When isPipelineStatus(status) is true and summary
extraction fails (currently only logger.warn is called), ensure the pipeline
step still writes a defensive fallback summary and emits the same persisted
event used for normal summaries so the step isn't silently lost; in the
AgentExecutor code path that uses isPipelineStatus, create a minimal summary
(e.g., "No summary generated for featureId <featureId> (status=<status>)") and
call the same persistence/emit routine the class uses for successful summaries
(the method in AgentExecutor that persists step output or emits pipeline events)
so featureId and status are recorded consistently instead of just logging via
logger.warn.
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 595-613: The code uses feature.summary.indexOf(headerWithNewline)
which can match a header string that appears inside free text; change the lookup
to only match real section boundaries by searching for the sequence separator +
headerWithNewline (or headerWithNewline at position 0) rather than any
occurrence: i.e., compute headerIndex by first searching
feature.summary.indexOf(separator + headerWithNewline) and, if not found, check
if feature.summary.startsWith(headerWithNewline) (use that index 0); then
proceed to compute afterHeader, nextSeparatorIndex and replace the slice using
feature.summary.substring as before, so stepHeader/stepSection/ separator only
affect actual section boundaries.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx`:
- Around line 106-114: The icon-only step navigation buttons (the Button
elements rendering <ChevronLeft /> and the corresponding right chevron) lack
accessible names; update the Button components that call onIndexChange (the left
one using onIndexChange(Math.max(0, activeIndex - 1)) and the right one that
advances the index) to include an explicit accessible label (e.g., add an
aria-label like "Previous step" and "Next step" or include visually-hidden text)
so assistive technologies announce their purpose while keeping the icons
visible.
- Around line 274-281: PhaseEntryCard currently receives an onClick handler
unconditionally which makes single-phase cards appear interactive but do
nothing; change the render to only pass onClick when navigation is possible by
conditionally providing onClick={hasMultiplePhases ? () =>
setActivePhaseIndex(index) : undefined} (keep existing props: entry, index,
totalPhases/phaseEntries.length, hasMultiplePhases, isActive using
hasMultiplePhases && index === activePhaseIndex). This ensures PhaseEntryCard's
interactive semantics only appear when hasMultiplePhases is true.
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`:
- Around line 113-121: The icon-only step navigation buttons in
agent-output-modal.tsx (the Button that renders <ChevronLeft ... /> and the
corresponding right-arrow Button with <ChevronRight ... />) lack accessible
names; update both Buttons used for step navigation (the ones calling
onIndexChange with Math.max(0, activeIndex - 1) and Math.min(totalSteps - 1,
activeIndex + 1)) to include descriptive accessible labels—e.g., add aria-label
attributes like "Previous step" and "Next step" or include visually hidden
text—so screen readers can announce their purpose while preserving the current
onClick, disabled, and size/variant props.
- Around line 201-205: getFirstNonEmptySummary can return string | null, but
parseAllPhaseSummaries and isAccumulatedSummary expect string | undefined;
normalize the nullable value before calling them by converting null to undefined
(e.g., use summary ?? undefined) so phaseEntries = useMemo(() =>
parseAllPhaseSummaries(summary ?? undefined), [summary]) and hasMultiplePhases =
useMemo(() => isAccumulatedSummary(summary ?? undefined), [summary]); update
references to summary passed into parseAllPhaseSummaries and
isAccumulatedSummary (and any other similar helper calls) to use this normalized
value.
---
Duplicate comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx`:
- Line 72: The task summary reset logic is turning empty/reset summaries into
null which then falls back to stale task.summary via the ?? operator; update the
handling in the taskSummaryMap update (where newMap.set(taskEvent.taskId, ...),
realtimeSummary, and event.summary are processed) to store an explicit empty
string or remove the map entry on reset instead of writing null, and adjust any
consumers that use taskSummaryMap (including the ?? fallback at the render site)
to treat empty string as an intentional reset (or check map.has(taskId) before
falling back to task.summary) so resets no longer display stale text; modify
setTaskSummaryMap and the code that computes the stored value accordingly.
---
Nitpick comments:
In `@apps/server/src/services/agent-executor-types.ts`:
- Around line 47-48: The optional property `status` currently typed as `string`
should be narrowed to the shared feature status type; replace `status?: string`
with `status?: FeatureStatusWithPipeline` (or `Feature['status']`) to enforce
valid statuses at compile time and import the `FeatureStatusWithPipeline` type
(or `Feature`) from its module; update the declaration where `status` is defined
so downstream code uses the stricter type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/server/src/services/agent-executor-types.tsapps/server/src/services/agent-executor.tsapps/server/src/services/auto-mode/facade.tsapps/server/src/services/feature-state-manager.tsapps/server/src/services/pipeline-orchestrator.tsapps/server/src/services/spec-parser.tsapps/server/tests/unit/services/feature-state-manager.test.tsapps/server/tests/unit/services/spec-parser.test.tsapps/server/tests/unit/types/pipeline-types.test.tsapps/server/tests/unit/ui/agent-output-summary-e2e.test.tsapps/server/tests/unit/ui/phase-summary-parser.test.tsapps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsxapps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsxapps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsxlibs/types/src/pipeline.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/server/src/services/spec-parser.ts
- apps/server/src/services/pipeline-orchestrator.ts
- apps/ui/src/components/views/board-view/dialogs/completed-features-modal.tsx
- apps/server/tests/unit/types/pipeline-types.test.ts
Comment on lines
+106
to
+114
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 w-7 p-0" | ||
| onClick={() => onIndexChange(Math.max(0, activeIndex - 1))} | ||
| disabled={activeIndex === 0} | ||
| > | ||
| <ChevronLeft className="w-4 h-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add accessible names to icon-only step navigation buttons.
The chevron-only buttons currently have no accessible label, so assistive tech will announce them as generic buttons.
♿ Suggested fix
<Button
variant="ghost"
size="sm"
className="h-7 w-7 p-0"
+ aria-label="Previous step"
onClick={() => onIndexChange(Math.max(0, activeIndex - 1))}
disabled={activeIndex === 0}
>
<ChevronLeft className="w-4 h-4" />
</Button>
@@
<Button
variant="ghost"
size="sm"
className="h-7 w-7 p-0"
+ aria-label="Next step"
onClick={() => onIndexChange(Math.min(phaseEntries.length - 1, activeIndex + 1))}
disabled={activeIndex === phaseEntries.length - 1}
>
<ChevronRight className="w-4 h-4" />
</Button>Also applies to: 133-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx`
around lines 106 - 114, The icon-only step navigation buttons (the Button
elements rendering <ChevronLeft /> and the corresponding right chevron) lack
accessible names; update the Button components that call onIndexChange (the left
one using onIndexChange(Math.max(0, activeIndex - 1)) and the right one that
advances the index) to include an explicit accessible label (e.g., add an
aria-label like "Previous step" and "Next step" or include visually-hidden text)
so assistive technologies announce their purpose while keeping the icons
visible.
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx
Show resolved
Hide resolved
Comment on lines
+113
to
+121
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-7 w-7 p-0" | ||
| onClick={() => onIndexChange(Math.max(0, activeIndex - 1))} | ||
| disabled={activeIndex === 0} | ||
| > | ||
| <ChevronLeft className="w-4 h-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add accessible labels to icon-only step navigation buttons.
At Line 113 and Line 140, the controls render only icons, so assistive tech gets unnamed buttons.
💡 Proposed fix
<Button
variant="ghost"
size="sm"
className="h-7 w-7 p-0"
+ aria-label="Previous step"
onClick={() => onIndexChange(Math.max(0, activeIndex - 1))}
disabled={activeIndex === 0}
>
@@
<Button
variant="ghost"
size="sm"
className="h-7 w-7 p-0"
+ aria-label="Next step"
onClick={() => onIndexChange(Math.min(phaseEntries.length - 1, activeIndex + 1))}
disabled={activeIndex === phaseEntries.length - 1}
>Also applies to: 140-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`
around lines 113 - 121, The icon-only step navigation buttons in
agent-output-modal.tsx (the Button that renders <ChevronLeft ... /> and the
corresponding right-arrow Button with <ChevronRight ... />) lack accessible
names; update both Buttons used for step navigation (the ones calling
onIndexChange with Math.max(0, activeIndex - 1) and Math.min(totalSteps - 1,
activeIndex + 1)) to include descriptive accessible labels—e.g., add aria-label
attributes like "Previous step" and "Next step" or include visually hidden
text—so screen readers can announce their purpose while preserving the current
onClick, disabled, and size/variant props.
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/feature-state-manager.ts (1)
711-725:⚠️ Potential issue | 🟡 MinorPersist empty task summaries consistently.
At Line 711,
if (summary)drops intentionally empty summaries (''), while Line 725 still emitssummary. That can desync persisted task state from emitted payload.🛠️ Suggested fix
- if (summary) { + if (summary !== undefined) { task.summary = summary; } @@ - summary, + ...(summary !== undefined ? { summary } : {}), tasks: feature.planSpec.tasks,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/feature-state-manager.ts` around lines 711 - 725, The code currently uses if (summary) before setting task.summary which drops intentionally empty summaries (''), causing persisted state to differ from the emitted payload; change the condition to check for undefined (e.g., if (summary !== undefined)) or set task.summary unconditionally when a summary argument is provided so that atomicWriteJson(featurePath, feature, ...) persists the same summary value that emitAutoModeEvent('auto_mode_task_status', { ..., summary, ... }) sends, and ensure feature.updatedAt and the call to atomicWriteJson remain unchanged.
♻️ Duplicate comments (2)
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
113-121:⚠️ Potential issue | 🟡 MinorAdd accessible labels to the icon-only step navigation buttons.
Both chevron buttons should have explicit accessible names.
♿ Suggested fix
<Button variant="ghost" size="sm" className="h-7 w-7 p-0" + aria-label="Previous step" onClick={() => onIndexChange(Math.max(0, activeIndex - 1))} disabled={activeIndex === 0} > @@ <Button variant="ghost" size="sm" className="h-7 w-7 p-0" + aria-label="Next step" onClick={() => onIndexChange(Math.min(phaseEntries.length - 1, activeIndex + 1))} disabled={activeIndex === phaseEntries.length - 1} >Also applies to: 140-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx` around lines 113 - 121, The icon-only step navigation Buttons (the one rendering <ChevronLeft className="w-4 h-4" /> and its counterpart with <ChevronRight ... />) lack accessible names; add explicit accessible labels by passing aria-label props (e.g., aria-label="Previous step" on the ChevronLeft Button and aria-label="Next step" on the ChevronRight Button) or include visually hidden text inside the Button to provide an accessible name while keeping the icon-only appearance (update the Button props in the agent-output-modal component where the onClick handlers call onIndexChange).apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx (1)
106-114:⚠️ Potential issue | 🟡 MinorAdd accessible names to the icon-only step navigation buttons.
The chevron-only buttons need explicit labels for screen readers.
♿ Suggested fix
<Button variant="ghost" size="sm" className="h-7 w-7 p-0" + aria-label="Previous step" onClick={() => onIndexChange(Math.max(0, activeIndex - 1))} disabled={activeIndex === 0} > @@ <Button variant="ghost" size="sm" className="h-7 w-7 p-0" + aria-label="Next step" onClick={() => onIndexChange(Math.min(phaseEntries.length - 1, activeIndex + 1))} disabled={activeIndex === phaseEntries.length - 1} >Also applies to: 133-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx` around lines 106 - 114, The icon-only navigation Buttons (the one rendering <ChevronLeft> and the corresponding <ChevronRight> button) lack accessible names; update the Button elements in summary-dialog.tsx (the Buttons that call onIndexChange(Math.max(0, activeIndex - 1)) and onIndexChange(Math.min(items.length - 1, activeIndex + 1))) to include explicit accessible labels (e.g., aria-label or title) such as "Previous card" and "Next card" so screen readers can announce their purpose, and ensure the disabled state still communicates appropriately.
🧹 Nitpick comments (1)
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx (1)
47-151: Consider extractingPhaseEntryCardandStepNavigatorinto a shared component.This logic is now duplicated with
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx, which increases drift risk for accessibility and behavior fixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx` around lines 47 - 151, Duplicate UI logic in PhaseEntryCard and StepNavigator should be extracted into a single shared component to avoid drift; create a new shared component (e.g., PhaseSteps or PhaseNavigator) that exports PhaseEntryCard and StepNavigator plus any shared types (PhaseSummaryEntry) and move the implementations from apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx into that file, then replace the local definitions in this file and in apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx with imports from the new shared module, making sure to preserve props (entry, index, totalPhases, hasMultiplePhases, isActive, onClick, phaseEntries, activeIndex, onIndexChange), accessibility props (role, tabIndex, onKeyDown), and styles/variants so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/services/agent-executor.ts`:
- Around line 383-395: The fallback summary may include the follow-up scaffold
because sessionContent is taken directly from responseText after
previousContent; update the fallback logic around sessionContent/extractSummary
so that before using fallback you strip or detect and remove any injected
scaffold markers (e.g., the follow-up header like '--- / ## Follow-up Session')
or other scaffold-only segments from sessionContent; specifically modify the
block that checks isPipelineStatus(status) and uses
callbacks.saveFeatureSummary(projectPath, featureId, fallback) to clean
sessionContent (or validate it contains real agent output) and only persist a
non-scaffold trimmed fallback, otherwise skip saving or try a different
fallback.
---
Outside diff comments:
In `@apps/server/src/services/feature-state-manager.ts`:
- Around line 711-725: The code currently uses if (summary) before setting
task.summary which drops intentionally empty summaries (''), causing persisted
state to differ from the emitted payload; change the condition to check for
undefined (e.g., if (summary !== undefined)) or set task.summary unconditionally
when a summary argument is provided so that atomicWriteJson(featurePath,
feature, ...) persists the same summary value that
emitAutoModeEvent('auto_mode_task_status', { ..., summary, ... }) sends, and
ensure feature.updatedAt and the call to atomicWriteJson remain unchanged.
---
Duplicate comments:
In
`@apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx`:
- Around line 106-114: The icon-only navigation Buttons (the one rendering
<ChevronLeft> and the corresponding <ChevronRight> button) lack accessible
names; update the Button elements in summary-dialog.tsx (the Buttons that call
onIndexChange(Math.max(0, activeIndex - 1)) and
onIndexChange(Math.min(items.length - 1, activeIndex + 1))) to include explicit
accessible labels (e.g., aria-label or title) such as "Previous card" and "Next
card" so screen readers can announce their purpose, and ensure the disabled
state still communicates appropriately.
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`:
- Around line 113-121: The icon-only step navigation Buttons (the one rendering
<ChevronLeft className="w-4 h-4" /> and its counterpart with <ChevronRight ...
/>) lack accessible names; add explicit accessible labels by passing aria-label
props (e.g., aria-label="Previous step" on the ChevronLeft Button and
aria-label="Next step" on the ChevronRight Button) or include visually hidden
text inside the Button to provide an accessible name while keeping the icon-only
appearance (update the Button props in the agent-output-modal component where
the onClick handlers call onIndexChange).
---
Nitpick comments:
In `@apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx`:
- Around line 47-151: Duplicate UI logic in PhaseEntryCard and StepNavigator
should be extracted into a single shared component to avoid drift; create a new
shared component (e.g., PhaseSteps or PhaseNavigator) that exports
PhaseEntryCard and StepNavigator plus any shared types (PhaseSummaryEntry) and
move the implementations from
apps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx into that
file, then replace the local definitions in this file and in
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx
with imports from the new shared module, making sure to preserve props (entry,
index, totalPhases, hasMultiplePhases, isActive, onClick, phaseEntries,
activeIndex, onIndexChange), accessibility props (role, tabIndex, onKeyDown),
and styles/variants so behavior remains identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/services/agent-executor.tsapps/server/src/services/feature-state-manager.tsapps/server/tests/unit/services/agent-executor-summary.test.tsapps/server/tests/unit/services/feature-state-manager.test.tsapps/server/tests/unit/ui/summary-normalization.test.tsapps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsxapps/ui/src/components/views/board-view/dialogs/agent-output-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/tests/unit/services/agent-executor-summary.test.ts
Collaborator
Author
|
@DhanushSantosh fyi Fyi what @gsxdsm ? |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/services/agent-executor.ts (1)
381-393: Consider centralizing scaffold format as a single constant.The strip regex and scaffold insertion format are tightly coupled; extracting one shared scaffold constant (and deriving stripping from it) will reduce drift risk during future edits.
♻️ Suggested refactor
export class AgentExecutor { private static readonly WRITE_DEBOUNCE_MS = 500; private static readonly STREAM_HEARTBEAT_MS = 15_000; + private static readonly FOLLOW_UP_SCAFFOLD = '\n\n---\n\n## Follow-up Session\n\n'; @@ - let responseText = previousContent - ? `${previousContent}\n\n---\n\n## Follow-up Session\n\n` - : ''; + let responseText = previousContent + ? `${previousContent}${AgentExecutor.FOLLOW_UP_SCAFFOLD}` + : ''; @@ - const scaffoldPattern = /^\s*---\s*##\s*Follow-up Session\s*/; - return content.replace(scaffoldPattern, ''); + if (content.startsWith(AgentExecutor.FOLLOW_UP_SCAFFOLD)) { + return content.slice(AgentExecutor.FOLLOW_UP_SCAFFOLD.length); + } + const scaffoldPattern = /^\s*---\s*##\s*Follow-up Session\s*/; + return content.replace(scaffoldPattern, ''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-executor.ts` around lines 381 - 393, Extract the literal scaffold into a single exported constant (e.g. FOLLOW_UP_SCAFFOLD) and replace the inline scaffoldPattern in stripFollowUpScaffold with a regex derived from that constant (escape the constant for regex and prefix with /^\s*/ to preserve existing whitespace handling); then update any places that insert the scaffold to use the same FOLLOW_UP_SCAFFOLD constant so both insertion and stripping share one source of truth (change references in stripFollowUpScaffold and the code paths that append/insert the scaffold).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/services/agent-executor.ts`:
- Around line 381-393: Extract the literal scaffold into a single exported
constant (e.g. FOLLOW_UP_SCAFFOLD) and replace the inline scaffoldPattern in
stripFollowUpScaffold with a regex derived from that constant (escape the
constant for regex and prefix with /^\s*/ to preserve existing whitespace
handling); then update any places that insert the scaffold to use the same
FOLLOW_UP_SCAFFOLD constant so both insertion and stripping share one source of
truth (change references in stripFollowUpScaffold and the code paths that
append/insert the scaffold).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/services/agent-executor.tsapps/server/tests/unit/services/agent-executor.test.ts
gsxdsm
added a commit
to gsxdsm/automaker
that referenced
this pull request
Feb 26, 2026
* Changes from fix/agent-output-summary-for-pipeline-steps * feat: Optimize pipeline summary extraction and fix regex vulnerability * fix: Use fallback summary for pipeline steps when extraction fails * fix: Strip follow-up session scaffold from pipeline step fallback summaries
gsxdsm
added a commit
that referenced
this pull request
Mar 3, 2026
* Changes from fix/agent-output-summary-for-pipeline-steps * feat: Optimize pipeline summary extraction and fix regex vulnerability * fix: Use fallback summary for pipeline steps when extraction fails * fix: Strip follow-up session scaffold from pipeline step fallback summaries
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
runAgentFncallback from PipelineOrchestrator through AgentExecutor.execute()Changes
AgentExecutor.execute()to accept and invokecallbacks.saveFeatureSummary()when provided, enabling summary capture for pipeline stepsPipelineOrchestrator.executePipeline()to passrunAgentFncallback containing summary-saving logic to AgentExecutorPipelineOrchestrator.executePipeline()→runAgentFn()→AgentExecutor.execute()→callbacks.saveFeatureSummary()→FeatureStateManager.saveFeatureSummary()auto_mode_summaryevent with feature contextSummary by CodeRabbit
New Features
Improvements
Tests
Chores