Add interactive publishing#35
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughBare ChangesGuided publish workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PublishCommand
participant GuidedPublish
participant PublishPlan
participant Workspace
participant Provider
Operator->>PublishCommand: run revpack publish
PublishCommand->>Workspace: load and validate bundle material
Workspace-->>GuidedPublish: findings, replies, summary, note, checkpoint state
GuidedPublish-->>PublishCommand: selected PublishSelection
PublishCommand->>PublishPlan: execute selection
PublishPlan->>Provider: publish selected review material
Provider-->>PublishPlan: publication results
PublishPlan->>Workspace: remove published drafts and refresh state
PublishPlan-->>Operator: progress and final result
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
Rename review.md to note.md to better communicate its purpose
4fc221e to
eb7f610
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/orchestration/orchestrator.ts (1)
589-600: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPasses through
threadIdswithout validating length/order againstcomments.This is the same ordering-contract gap flagged in
provider.ts—publishReviewBatchforwards whateverthreadIdsthe provider returns straight through to callers that map it positionally to findings. Consider validatingthreadIds.length === comments.lengthhere (or at least logging a warning on mismatch) so a provider that returns partial/misordered IDs fails loudly instead of causing silent misattribution downstream inpublish-plan.ts.🤖 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/orchestration/orchestrator.ts` around lines 589 - 600, Validate the provider result in the publishReviewBatch flow before assigning threadIds, ensuring the returned IDs match comments.length and their positional ordering contract. On mismatch, fail loudly or emit the established warning and avoid passing invalid IDs to downstream callers; preserve the existing return shape for valid results.src/orchestration/publish-plan.ts (1)
214-226: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPositional
threadIdsmapping assumes provider preserves submission order.
threadIds[index]is matched tofindings[index]purely by array position. If a provider ever returns thread IDs in a different order than submitted (e.g., sorted by creation/ID rather than payload order), a finding's trackedproviderThreadIdwould silently point to the wrong thread. Thetypeof providerThreadId === 'string'guard only protects against missing entries, not misordering.🤖 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/orchestration/publish-plan.ts` around lines 214 - 226, Replace the positional threadIds[index] association in the findings cleanup loop with an order-independent mapping using the provider’s stable finding/thread correlation data. Ensure each finding receives only its own providerThreadId when calling orchestrator.workspace.appendPublishedAction, while preserving the existing behavior for missing IDs.
🤖 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/cli/commands/publish-tui.ts`:
- Around line 668-674: Update the finding label mapping to use each finding’s
`newPath` with a fallback to `oldPath` before rendering the path and position.
Preserve the existing line selection, selection markers, focus handling, and
label formatting.
- Around line 379-415: Update wrapText, fitColumn, and truncateColumn to measure
and slice text by grapheme clusters and terminal display width rather than
JavaScript code units, preserving ANSI handling and column limits for emoji and
CJK characters. Reuse or add a shared display-width helper near visibleText, and
add regression coverage for wrapping and truncating emoji/CJK content without
splitting graphemes or exceeding the requested width.
In `@src/cli/commands/publish.ts`:
- Line 693: Update the repository selection in the publish flow and
`publishAllPending` to use the active bundle’s repository first, falling back to
`getRepository()` only when the bundle target is unavailable; then pass that
result to `requirePublishRepository`. Ensure Git-derived repository selection
cannot override the loaded bundle’s authoritative publishing target.
- Around line 310-315: Update readOptionalTextFile to return an absent result
only when fs.readFile fails with an ENOENT error; rethrow permission, directory,
and other I/O errors while preserving their original cause. Keep the successful
read behavior and returned object shape unchanged.
In `@src/workspace/publish-material.ts`:
- Around line 83-108: The compare-and-mutate flows in the queue cleanup function
and clearPublishedDocument must hold the exclusive workspace/output lock across
the expected-content re-read and subsequent write, rename, or fs.rm operation.
Acquire the existing lock before validation, keep it held through mutation, and
release it reliably afterward while preserving the current mismatch errors and
return behavior.
- Around line 359-364: Update loadPublishMaterial to resolve the note path using
DEFAULT_PUBLISH_PATHS.note instead of bundleState.outputs.review.path, while
retaining the summary resolution. Add coverage for an older bundle whose review
output references review.md and verify that legacy note content is not
published.
In `@src/workspace/workspace-manager.ts`:
- Around line 42-43: Update the discard-only filename list used by
discardOutputs() to include legacy review.md, without restoring its status or
publishing compatibility. In src/workspace/workspace-manager.ts lines 42-43, add
review.md to OUTPUT_FILENAMES; update src/workspace/workspace-manager.test.ts
lines 2168-2175 to assert that both note.md and review.md are removed.
---
Nitpick comments:
In `@src/orchestration/orchestrator.ts`:
- Around line 589-600: Validate the provider result in the publishReviewBatch
flow before assigning threadIds, ensuring the returned IDs match comments.length
and their positional ordering contract. On mismatch, fail loudly or emit the
established warning and avoid passing invalid IDs to downstream callers;
preserve the existing return shape for valid results.
In `@src/orchestration/publish-plan.ts`:
- Around line 214-226: Replace the positional threadIds[index] association in
the findings cleanup loop with an order-independent mapping using the provider’s
stable finding/thread correlation data. Ensure each finding receives only its
own providerThreadId when calling orchestrator.workspace.appendPublishedAction,
while preserving the existing behavior for missing IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2f3c476-56d5-40f8-9c73-254f3f100e1d
📒 Files selected for processing (29)
CHANGELOG.mdREADME.mddocs/adr/0002-agent-output-files-are-lazy-drafts.mddocs/commands.mdsrc/cli/commands/publish-tui.test.tssrc/cli/commands/publish-tui.tssrc/cli/commands/publish.test.tssrc/cli/commands/publish.tssrc/cli/commands/status.test.tssrc/cli/commands/status.tssrc/cli/index.tssrc/core/types.tssrc/orchestration/orchestrator.test.tssrc/orchestration/orchestrator.tssrc/orchestration/publish-plan.test.tssrc/orchestration/publish-plan.tssrc/providers/local/local-git-provider.test.tssrc/providers/provider.tssrc/workspace/finding-validator.test.tssrc/workspace/finding-validator.tssrc/workspace/publish-material.test.tssrc/workspace/publish-material.tssrc/workspace/workspace-manager.test.tssrc/workspace/workspace-manager.tstemplates/CONTEXT.md.hbstemplates/INSTRUCTIONS.mdtemplates/instructions/01-review-workflow-and-outputs.mdtemplates/instructions/05-review-note.mdtemplates/instructions/07-final-checks.md
| function wrapText(text: string, width: number): string[] { | ||
| const safeWidth = Math.max(10, width); | ||
| const result: string[] = []; | ||
| for (const sourceLine of text.replace(/\r\n/g, '\n').split('\n')) { | ||
| if (sourceLine.length <= safeWidth) { | ||
| result.push(sourceLine); | ||
| continue; | ||
| } | ||
| let remaining = sourceLine; | ||
| while (remaining.length > safeWidth) { | ||
| const candidate = remaining.slice(0, safeWidth + 1); | ||
| const breakAt = candidate.lastIndexOf(' '); | ||
| const end = breakAt > 0 ? breakAt : safeWidth; | ||
| result.push(remaining.slice(0, end)); | ||
| remaining = remaining.slice(end).replace(/^ /, ''); | ||
| } | ||
| result.push(remaining); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| const ANSI_PATTERN = new RegExp(`${String.fromCharCode(27)}\\[[0-?]*[ -/]*[@-~]`, 'g'); | ||
|
|
||
| function visibleText(value: string): string { | ||
| return value.replace(ANSI_PATTERN, ''); | ||
| } | ||
|
|
||
| function fitColumn(value: string, width: number): string { | ||
| const visible = visibleText(value); | ||
| if (visible.length > width) return visible.slice(0, Math.max(0, width - 1)) + (width > 0 ? '…' : ''); | ||
| return value + ' '.repeat(width - visible.length); | ||
| } | ||
|
|
||
| function truncateColumn(value: string, width: number): string { | ||
| const visible = visibleText(value); | ||
| if (visible.length <= width) return value; | ||
| return visible.slice(0, Math.max(0, width - 1)) + (width > 0 ? '…' : ''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file first, then inspect the relevant slice and search for any existing
# display-width / grapheme helpers in the repo.
ast-grep outline src/cli/commands/publish-tui.ts --view expanded
printf '\n--- targeted slice ---\n'
sed -n '350,440p' src/cli/commands/publish-tui.ts
printf '\n--- related usages of wrapText / fitColumn / truncateColumn ---\n'
rg -n "wrapText|fitColumn|truncateColumn|visibleText|ANSI_PATTERN" src/cli/commands/publish-tui.ts
printf '\n--- existing width/grapheme helpers in src ---\n'
rg -n "string-width|strip-ansi|grapheme|display width|wcwidth|width" srcRepository: stefanvictora/revpack
Length of output: 15808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- package manifests mentioning width/ansi/grapheme helpers ---\n'
rg -n '"(string-width|strip-ansi|grapheme-splitter|east-asian-width|wcwidth|emoji-regex)"' package.json package-lock.json pnpm-lock.yaml yarn.lock npm-shrinkwrap.json 2>/dev/null || true
printf '\n--- package.json ---\n'
cat -n package.jsonRepository: stefanvictora/revpack
Length of output: 4096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'src/cli/commands/*test.ts' 'src/cli/commands/*spec.ts' 'src/cli/commands/*.ts' | sed -n '1,120p'Repository: stefanvictora/revpack
Length of output: 658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/cli/commands/publish-tui.test.ts --view expanded
printf '\n--- focused test slices ---\n'
sed -n '1,260p' src/cli/commands/publish-tui.test.ts
printf '\n--- width-related assertions in publish-tui tests ---\n'
rg -n "wrapText|truncateColumn|fitColumn|unicode|emoji|CJK|column|width|ellipsis" src/cli/commands/publish-tui.test.tsRepository: stefanvictora/revpack
Length of output: 9390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- publish-tui.test.ts around rendering/layout tests ---\n'
sed -n '440,860p' src/cli/commands/publish-tui.test.ts
printf '\n--- direct references to unicode/ansi handling in tests ---\n'
rg -n "stripVTControlCharacters|emoji|CJK|Unicode|ansi|width|wrapText|fitColumn|truncateColumn" src/cli/commands/publish-tui.test.tsRepository: stefanvictora/revpack
Length of output: 15104
Make wrapping and truncation terminal-width aware. wrapText, fitColumn, and truncateColumn still operate on code units, so emoji/CJK can be split or spill past the allotted columns; use a grapheme/display-width helper here and add an emoji/CJK regression test.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 399-399: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${String.fromCharCode(27)}\\[[0-?]*[ -/]*[@-~], 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 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/cli/commands/publish-tui.ts` around lines 379 - 415, Update wrapText,
fitColumn, and truncateColumn to measure and slice text by grapheme clusters and
terminal display width rather than JavaScript code units, preserving ANSI
handling and column limits for emoji and CJK characters. Reuse or add a shared
display-width helper near visibleText, and add regression coverage for wrapping
and truncating emoji/CJK content without splitting graphemes or exceeding the
requested width.
Source: Coding guidelines
| ...model.findings.map(({ index, value }) => { | ||
| const position = value.newLine ?? value.oldLine ?? '?'; | ||
| const label = value.body.split('\n')[0] || '(untitled finding)'; | ||
| return ` ${focus?.kind === 'finding' && focus.index === index ? '> ' : ' '}${ | ||
| selection.findingIndexes.has(index) ? '[x]' : '[ ]' | ||
| } ${value.newPath}:${position} ${label}`; | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back to oldPath for deleted-file findings.
The downstream publish plan uses newPath || oldPath, but this list always renders newPath. Findings for deleted files can therefore appear as :line, making them difficult to identify.
Proposed fix
...model.findings.map(({ index, value }) => {
const position = value.newLine ?? value.oldLine ?? '?';
+ const displayPath = value.newPath || value.oldPath;
const label = value.body.split('\n')[0] || '(untitled finding)';
return ` ${focus?.kind === 'finding' && focus.index === index ? '> ' : ' '}${
selection.findingIndexes.has(index) ? '[x]' : '[ ]'
- } ${value.newPath}:${position} ${label}`;
+ } ${displayPath}:${position} ${label}`;
}),📝 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.
| ...model.findings.map(({ index, value }) => { | |
| const position = value.newLine ?? value.oldLine ?? '?'; | |
| const label = value.body.split('\n')[0] || '(untitled finding)'; | |
| return ` ${focus?.kind === 'finding' && focus.index === index ? '> ' : ' '}${ | |
| selection.findingIndexes.has(index) ? '[x]' : '[ ]' | |
| } ${value.newPath}:${position} ${label}`; | |
| }), | |
| ...model.findings.map(({ index, value }) => { | |
| const position = value.newLine ?? value.oldLine ?? '?'; | |
| const displayPath = value.newPath || value.oldPath; | |
| const label = value.body.split('\n')[0] || '(untitled finding)'; | |
| return ` ${focus?.kind === 'finding' && focus.index === index ? '> ' : ' '}${ | |
| selection.findingIndexes.has(index) ? '[x]' : '[ ]' | |
| } ${displayPath}:${position} ${label}`; | |
| }), |
🤖 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/cli/commands/publish-tui.ts` around lines 668 - 674, Update the finding
label mapping to use each finding’s `newPath` with a fallback to `oldPath`
before rendering the path and position. Preserve the existing line selection,
selection markers, focus handling, and label formatting.
| async function readOptionalTextFile(filePath: string): Promise<{ exists: boolean; content: string }> { | ||
| try { | ||
| return { exists: true, content: await fs.readFile(workspacePath(filePath), 'utf-8') }; | ||
| } catch { | ||
| return { exists: false, content: '' }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not treat every note read failure as a missing file.
Permission, directory, and I/O errors currently become an empty note, hiding the real failure. Only ENOENT should return { exists: false }; rethrow other errors with their cause.
Proposed fix
async function readOptionalTextFile(filePath: string): Promise<{ exists: boolean; content: string }> {
try {
return { exists: true, content: await fs.readFile(workspacePath(filePath), 'utf-8') };
- } catch {
- return { exists: false, content: '' };
+ } catch (error) {
+ if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
+ return { exists: false, content: '' };
+ }
+ throw new Error(`Could not read ${filePath}.`, { cause: error });
}
}📝 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.
| async function readOptionalTextFile(filePath: string): Promise<{ exists: boolean; content: string }> { | |
| try { | |
| return { exists: true, content: await fs.readFile(workspacePath(filePath), 'utf-8') }; | |
| } catch { | |
| return { exists: false, content: '' }; | |
| } | |
| async function readOptionalTextFile(filePath: string): Promise<{ exists: boolean; content: string }> { | |
| try { | |
| return { exists: true, content: await fs.readFile(workspacePath(filePath), 'utf-8') }; | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | |
| return { exists: false, content: '' }; | |
| } | |
| throw new Error(`Could not read ${filePath}.`, { cause: error }); | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 311-311: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(workspacePath(filePath), 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli/commands/publish.ts` around lines 310 - 315, Update
readOptionalTextFile to return an absent result only when fs.readFile fails with
an ENOENT error; rethrow permission, directory, and other I/O errors while
preserving their original cause. Keep the successful read behavior and returned
object shape unchanged.
|
|
||
| let material = await loadMaterial(process.cwd()); | ||
| const orchestrator = await createPublishOrchestrator(); | ||
| const repository = requirePublishRepository((await getRepository()) ?? material.bundleState.target.repository); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the active bundle’s repository before inspecting Git remotes.
getRepoFromGit() selects the first remote, so repositories with multiple remotes can publish or perform freshness checks against a target different from the loaded bundle. The active bundle is the authoritative publishing target; use Git only as a fallback.
Proposed fix
- const repository = requirePublishRepository((await getRepository()) ?? material.bundleState.target.repository);
+ const repository = requirePublishRepository(
+ material.bundleState.target.repository || (await getRepository()),
+ );Apply the equivalent ordering in publishAllPending.
Also applies to: 754-754
🤖 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/cli/commands/publish.ts` at line 693, Update the repository selection in
the publish flow and `publishAllPending` to use the active bundle’s repository
first, falling back to `getRepository()` only when the bundle target is
unavailable; then pass that result to `requirePublishRepository`. Ensure
Git-derived repository selection cannot override the loaded bundle’s
authoritative publishing target.
| if (options?.expectedEntries !== undefined) { | ||
| const currentEntries = await readOptionalJsonArray(filePath, 'draft objects'); | ||
| if (!isDeepStrictEqual(currentEntries, options.expectedEntries)) { | ||
| throw new Error(`${filePath} changed after publish material was loaded; the newer queue was left unchanged.`); | ||
| } | ||
| } | ||
| if (retained.length === 0 && options?.deleteWhenEmpty) { | ||
| await fs.rm(filePath, { force: true }); | ||
| return true; | ||
| } | ||
| await writeJsonArrayAtomically( | ||
| filePath, | ||
| retained.map((draft) => draft.raw), | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| export async function clearPublishedDocument(filePath: string, expectedContent?: string): Promise<void> { | ||
| if (expectedContent !== undefined) { | ||
| const currentContent = await readOptionalFile(filePath); | ||
| if (currentContent !== expectedContent) { | ||
| throw new Error(`${filePath} changed after publish material was loaded; the newer document was left unchanged.`); | ||
| } | ||
| } | ||
| await fs.rm(filePath, { force: true }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the compare-and-mutate operations atomic.
Another process can update a queue or note after the equality check but before rename/rm; this then overwrites or deletes the newer draft despite the preservation guard. Hold an exclusive workspace/output lock across the re-read and mutation for both queue and document cleanup.
🤖 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/workspace/publish-material.ts` around lines 83 - 108, The
compare-and-mutate flows in the queue cleanup function and
clearPublishedDocument must hold the exclusive workspace/output lock across the
expected-content re-read and subsequent write, rename, or fs.rm operation.
Acquire the existing lock before validation, keep it held through mutation, and
release it reliably afterward while preserving the current mismatch errors and
return behavior.
| export async function loadPublishMaterial(workingDir: string): Promise<PublishMaterial> { | ||
| const bundleState = await loadBundleState(workingDir); | ||
| const [summaryPath, notePath] = await Promise.all([ | ||
| resolveBundleOutputPath(workingDir, bundleState.outputs?.summary?.path, 'Summary'), | ||
| resolveBundleOutputPath(workingDir, bundleState.outputs?.review?.path, 'Review note'), | ||
| ]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not load legacy review.md from an older bundle contract.
Guided publishing and publish all still resolve bundleState.outputs.review.path. An existing bundle whose path is .revpack/outputs/review.md will therefore publish that legacy draft, while the explicit publish note path correctly ignores it. Resolve the note from DEFAULT_PUBLISH_PATHS.note, and add coverage for an old bundle referencing review.md.
Proposed fix
const [summaryPath, notePath] = await Promise.all([
resolveBundleOutputPath(workingDir, bundleState.outputs?.summary?.path, 'Summary'),
- resolveBundleOutputPath(workingDir, bundleState.outputs?.review?.path, 'Review note'),
+ resolveBundleOutputPath(workingDir, DEFAULT_PUBLISH_PATHS.note, 'Review note'),
]);📝 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.
| export async function loadPublishMaterial(workingDir: string): Promise<PublishMaterial> { | |
| const bundleState = await loadBundleState(workingDir); | |
| const [summaryPath, notePath] = await Promise.all([ | |
| resolveBundleOutputPath(workingDir, bundleState.outputs?.summary?.path, 'Summary'), | |
| resolveBundleOutputPath(workingDir, bundleState.outputs?.review?.path, 'Review note'), | |
| ]); | |
| export async function loadPublishMaterial(workingDir: string): Promise<PublishMaterial> { | |
| const bundleState = await loadBundleState(workingDir); | |
| const [summaryPath, notePath] = await Promise.all([ | |
| resolveBundleOutputPath(workingDir, bundleState.outputs?.summary?.path, 'Summary'), | |
| resolveBundleOutputPath(workingDir, DEFAULT_PUBLISH_PATHS.note, 'Review note'), | |
| ]); |
🤖 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/workspace/publish-material.ts` around lines 359 - 364, Update
loadPublishMaterial to resolve the note path using DEFAULT_PUBLISH_PATHS.note
instead of bundleState.outputs.review.path, while retaining the summary
resolution. Add coverage for an older bundle whose review output references
review.md and verify that legacy note content is not published.
| const OUTPUT_FILENAMES = ['replies.json', 'new-findings.json', 'summary.md', 'note.md'] as const; | ||
| const DEFAULT_REVIEW_NOTE_PATH = '.revpack/outputs/note.md'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Discard legacy review drafts instead of preserving them.
prepare --discard-outputs calls discardOutputs(), but this list now leaves an existing review.md behind. An explicit discard can therefore retain stale or sensitive review content after refresh. Delete legacy drafts without restoring their status or publishing compatibility.
src/workspace/workspace-manager.ts#L42-L43: includereview.mdin the discard-only filename list.src/workspace/workspace-manager.test.ts#L2168-L2175: expect bothnote.mdand legacyreview.mdto be removed.
📍 Affects 2 files
src/workspace/workspace-manager.ts#L42-L43(this comment)src/workspace/workspace-manager.test.ts#L2168-L2175
🤖 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/workspace/workspace-manager.ts` around lines 42 - 43, Update the
discard-only filename list used by discardOutputs() to include legacy review.md,
without restoring its status or publishing compatibility. In
src/workspace/workspace-manager.ts lines 42-43, add review.md to
OUTPUT_FILENAMES; update src/workspace/workspace-manager.test.ts lines 2168-2175
to assert that both note.md and review.md are removed.
1 participant
Rename review.md to note.md to better communicate its purpose
Closes #29
Added
revpack publishflow for selecting pending review outputs and checkpoint publishing interactively.revpack publish noteandnote.mdas the primary review-note publishing path, while keepingpublish reviewas a hidden command alias.Changed
note.mdinstead ofreview.md.review.mddrafts from refreshed bundles.Summary by CodeRabbit
New Features
revpack publishworkflow, with keyboard navigation, selective publishing, previews, and confirmation.note.mdand added therevpack publish notecommand.Bug Fixes
Documentation
note.md.