fix: validate artifact content before marking workflow step done#1098
Open
mvanhorn wants to merge 2 commits into
Open
fix: validate artifact content before marking workflow step done#1098mvanhorn wants to merge 2 commits into
mvanhorn wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR changes artifact completion detection to require generated outputs contain meaningful content (not empty, whitespace-only, or comment-only), ensuring workflow resumes regenerate incomplete artifact files instead of treating them as completed. ChangesArtifact Content Validation for Workflow Resume
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.changeset/validate-artifact-content.md:
- Line 7: Update the changeset note sentence so it explicitly mentions
"comment-only" artifact files in addition to empty or whitespace-only files;
change the phrasing in the existing summary line (the sentence that currently
reads "Fixed workflow resumes so empty or whitespace-only artifact files are
regenerated instead of being treated as completed.") to include a short clause
like "and files containing only comments (e.g., HTML comment-only files)" so the
note matches the actual behavior.
In `@src/core/artifact-graph/outputs.ts`:
- Around line 56-59: artifactOutputComplete currently triggers
resolveArtifactOutputs twice via artifactOutputExists and
artifactOutputContentValid; change it to call resolveArtifactOutputs once, store
the resolved outputs, and use those results to perform the existence and
content-validity checks. Either refactor artifactOutputExists and
artifactOutputContentValid to accept the pre-resolved outputs (e.g., add
parameters like resolvedOutputs) or implement the existence/content checks
directly inside artifactOutputComplete using resolveArtifactOutputs' return;
reference artifactOutputComplete, artifactOutputExists,
artifactOutputContentValid, and resolveArtifactOutputs when updating code so you
only resolve once and reuse the resolved list for both checks.
In `@test/core/artifact-graph/outputs.test.ts`:
- Around line 46-58: Add a new unit test in outputs.test.ts that writes
HTML-comment-only content into the same temp file used by the other cases (e.g.,
write only an HTML comment like <!-- note --> to proposal.md in tempDir) and
assert that artifactOutputContentValid(tempDir, 'proposal.md') === false and
artifactOutputComplete(tempDir, 'proposal.md') === false; place this test
alongside the existing whitespace-only and heading tests so the helper-level
contract for comment-only content is explicitly covered.
In `@test/core/artifact-graph/state.test.ts`:
- Around line 90-101: Add a negative test case to ensure files that contain only
HTML comments are not treated as completed: in
test/core/artifact-graph/state.test.ts add an it(...) similar to the existing
heading test that uses createSchema and ArtifactGraph.fromSchema to create the
graph, writes a file at path.join(tempDir, 'proposal.md') containing only HTML
comments (e.g. "<!-- comment -->\n") via fs.writeFileSync, calls
detectCompleted(graph, tempDir), and asserts
expect(completed.has('proposal')).toBe(false); this mirrors the existing
single-heading test but verifies the "non-comment meaningful content" rule.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1d893d1-6079-4df1-ba92-cf0de9c45737
📒 Files selected for processing (5)
.changeset/validate-artifact-content.mdsrc/core/artifact-graph/outputs.tssrc/core/artifact-graph/state.tstest/core/artifact-graph/outputs.test.tstest/core/artifact-graph/state.test.ts
…tests - artifactOutputComplete now resolves outputs once instead of twice - changeset note mentions comment-only files (matches impl behavior) - outputs.test.ts: comment-only artifact rejected by content validator - state.test.ts: detectCompleted excludes comment-only files Tests: pnpm vitest run test/core/artifact-graph/ — 147 passed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/core/artifact-graph/outputs.ts`:
- Around line 67-73: The current check uses trimmed.startsWith('<!--') so
multi-line HTML comments like <!--\nfoo\n--> cause the middle lines (e.g. "foo")
to be treated as content; update the logic that determines "meaningful" content
by either stripping all HTML comment blocks from content before splitting (use a
non-greedy block removal for <!-- ... -->) or by iterating lines with an
inComment boolean that flips when encountering <!-- and --> so lines inside a
comment are ignored; adjust the code that uses content.split(...).some(...) (and
the trimmed variable/startsWith('<!--') test) to use this new comment-aware
approach so only real non-comment, non-whitespace lines count as meaningful
content.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 985611bb-85fe-48b1-8c7e-dfddfc6cb0b2
📒 Files selected for processing (4)
.changeset/validate-artifact-content.mdsrc/core/artifact-graph/outputs.tstest/core/artifact-graph/outputs.test.tstest/core/artifact-graph/state.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/validate-artifact-content.md
Comment on lines
+67
to
+73
| return content | ||
| .split(/\r?\n/) | ||
| .some((line) => { | ||
| const trimmed = line.trim(); | ||
|
|
||
| return trimmed.length > 0 && !trimmed.startsWith('<!--'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Multi-line HTML comment blocks are incorrectly treated as meaningful content.
On Line 67–73, comment detection only checks trimmed.startsWith('<!--'). For block comments like <!--\ncomment\n-->, the middle line (comment) is currently counted as valid content, so comment-only files can be marked complete.
💡 Proposed fix
function isArtifactOutputFileContentValid(filePath: string): boolean {
try {
const content = fs.readFileSync(filePath, 'utf8');
-
- return content
- .split(/\r?\n/)
- .some((line) => {
- const trimmed = line.trim();
-
- return trimmed.length > 0 && !trimmed.startsWith('<!--');
- });
+ const withoutHtmlComments = content.replace(/<!--[\s\S]*?(-->|$)/g, '');
+ return withoutHtmlComments
+ .split(/\r?\n/)
+ .some((line) => line.trim().length > 0);
} catch {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return content | |
| .split(/\r?\n/) | |
| .some((line) => { | |
| const trimmed = line.trim(); | |
| return trimmed.length > 0 && !trimmed.startsWith('<!--'); | |
| }); | |
| function isArtifactOutputFileContentValid(filePath: string): boolean { | |
| try { | |
| const content = fs.readFileSync(filePath, 'utf8'); | |
| const withoutHtmlComments = content.replace(/<!--[\s\S]*?(-->|$)/g, ''); | |
| return withoutHtmlComments | |
| .split(/\r?\n/) | |
| .some((line) => line.trim().length > 0); | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 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/core/artifact-graph/outputs.ts` around lines 67 - 73, The current check
uses trimmed.startsWith('<!--') so multi-line HTML comments like <!--\nfoo\n-->
cause the middle lines (e.g. "foo") to be treated as content; update the logic
that determines "meaningful" content by either stripping all HTML comment blocks
from content before splitting (use a non-greedy block removal for <!-- ... -->)
or by iterating lines with an inComment boolean that flips when encountering
<!-- and --> so lines inside a comment are ignored; adjust the code that uses
content.split(...).some(...) (and the trimmed variable/startsWith('<!--') test)
to use this new comment-aware approach so only real non-comment, non-whitespace
lines count as meaningful content.
Author
|
Addressed in ce0c1d7:
Tests: |
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
Workflow interruptions (network blip, Ctrl-C, agent timeout) leave partially-written artifact files behind. On resume,
detectCompletedseesproposal.mdexists and marks the artifactdone, so the workflow skips re-generating it. The half-written file then flows into downstream tasks, degrading every subsequent artifact silently.Add a content-validation layer in
artifact-graph/outputs.ts:artifactOutputContentValid(changeDir, generates)— every resolved output file has at least one non-blank, non-HTML-comment lineartifactOutputComplete(changeDir, generates)— composesartifactOutputExistswith content validationSwitch
isArtifactCompleteinstate.tsto useartifactOutputComplete. KeepartifactOutputExistsandresolveArtifactOutputssemantics unchanged so other callers (applyinstructions, glob resolution) aren't affected.Why this matters
Reporter @jiehu03 in #1084 identified the root cause precisely:
detectCompleted->isArtifactComplete->artifactOutputExistsonly check file existence (fs.statSync/fast-glob). An empty file and a complete file produce identical "done" verdicts; there's no warning, no diff, no error — only a half-written artifact silently flowing into the next step.Tests: 7 new cases under
test/core/artifact-graph/cover empty, whitespace-only, comment-only, valid content, and glob-pattern scenarios.pnpm vitest run test/core/artifact-graph/-> 145 passed.Fixes #1084
AI was used for assistance.
Summary by CodeRabbit