fix: validate artifact content before marking workflow step done#1098
fix: validate artifact content before marking workflow step done#1098mvanhorn wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 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}
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.
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
|
Addressed in ce0c1d7:
Tests: |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
This is the right fix shape, and I like that artifactOutputExists stays existence-only while completion gets the stricter check. I ran the artifact-graph tests on the PR branch and they pass: pnpm vitest run test/core/artifact-graph/ → 147 passed.
One blocker remains from the latest CodeRabbit note: multiline HTML comment-only files still count as meaningful content. I added a temporary regression test with <!--\nplaceholder\n-->, and artifactOutputContentValid(tempDir, 'proposal.md') returned true. The current line-based trimmed.startsWith('<!--') only skips the opening line, so the middle comment line is treated as real content.
Please strip HTML comment blocks before checking lines, or track an inComment state across lines, and add that regression test. I would not block this PR on full template/schema-aware validation yet. This is a good narrow guard once comment-only detection is actually robust, but the release note/body should avoid implying it catches every possible partially-written artifact, since a heading-only partial still passes.
Multi-line HTML comments like:
<!--
placeholder
-->
were incorrectly treated as meaningful content because the line-based
check only skipped lines starting with '<!--'. The middle line
("placeholder") was counted as real content, so comment-only files
were marked complete.
Strip HTML comment blocks via /<!--[\s\S]*?(-->|$)/g before splitting
into lines, then any non-whitespace line counts as meaningful content.
The non-greedy (-->|$) tail handles unterminated comment blocks safely.
Add regression tests in outputs.test.ts and state.test.ts covering the
multi-line comment-only case for both artifactOutputContentValid /
artifactOutputComplete and detectCompleted.
Update the changeset to mention HTML-comment-only specifically and
note that heading-only or other partially-written artifacts are not
yet detected (heading content still counts as meaningful).
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Fixed in e608230. Applied CodeRabbit's suggested approach: strip HTML comment blocks via Added regression tests in both layers:
Updated the changeset wording to specify "HTML-comment-only" and added the heading-only caveat you flagged, so the note doesn't overpromise. Local |
2 participants
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
Bug Fixes
Tests
Documentation