Improve sdpub archive exploration for agents#58
Merged
Conversation
Summary by CodeRabbit
WalkthroughThis PR replaces the older Possibly related PRs
✨ Finishing Touches✨ Simplify code
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/args.ts (2)
435-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsupported globally-registered flags in each command parser.
These parser branches don’t reject all globally-known options, so flags like
--id,--cursor,--type,--order,--match,--chapter,--confirm,--budget, and--tocan be parsed and then silently ignored depending on command. That hides user mistakes instead of failing fast.Also applies to: 1146-1152, 1632-1641
🤖 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/args.ts` around lines 435 - 439, The parser branches are not rejecting all globally-known options, allowing flags like --id, --cursor, --type, --order, --match, --chapter, --to to be silently ignored instead of failing fast. In src/cli/args.ts at lines 435-439, add rejectConvertFlag calls for all missing globally-known flags (--id, --cursor, --type, --order, --match, --chapter, --to) in addition to the existing rejectConvertFlag calls for budget, confirm, and json. Apply the same set of rejectConvertFlag calls to the other affected sites at lines 1146-1152 and 1632-1641 to ensure consistent validation across all command parser branches.
2093-2114:⚠️ Potential issue | 🟠 MajorRemove stale
fragmentsacceptance fromisArchiveAction.
isArchiveActioncurrently returns true for"fragments", but that action is not part ofCLIArchiveActionand has no branch inparseArchiveArguments. This can route invalid input into an internal parser failure path instead of a clean unknown-command error.Suggested fix
function isArchiveAction(value: string | undefined): value is CLIArchiveAction { return ( value === "backlinks" || value === "build" || value === "estimate" || - value === "fragments" || value === "export" ||🤖 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/args.ts` around lines 2093 - 2114, Remove the stale `"fragments"` string comparison from the isArchiveAction type guard function. The `"fragments"` action is not a valid member of the CLIArchiveAction type and has no corresponding handler in parseArchiveArguments, so accepting it as valid causes invalid input to route into an internal parser failure instead of producing a clean unknown-command error. Simply delete the line checking `value === "fragments" ||` from the function's conditional chain.
🧹 Nitpick comments (1)
test/cli/main.test.ts (1)
165-165: ⚡ Quick winStrengthen negative-dispatch assertions for all maintenance handlers.
At Line 165, Line 182, Line 208, and Line 251, the tests only verify
archiveMetaRunCallsis untouched. Add zero-call checks forarchiveCoverRunCallsandarchiveChapterRunCallstoo, so accidental misrouting in non-maintenance paths is caught.Suggested assertion additions
- expect(mainMockState.archiveMetaRunCalls).toHaveLength(0); + expect(mainMockState.archiveMetaRunCalls).toHaveLength(0); + expect(mainMockState.archiveCoverRunCalls).toHaveLength(0); + expect(mainMockState.archiveChapterRunCalls).toHaveLength(0);Also applies to: 182-182, 208-208, 251-251
🤖 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 `@test/cli/main.test.ts` at line 165, Strengthen the negative-dispatch assertions at all four locations (lines 165, 182, 208, and 251) by adding checks for archiveCoverRunCalls and archiveChapterRunCalls in addition to the existing archiveMetaRunCalls checks. For each location, after the existing expect(mainMockState.archiveMetaRunCalls).toHaveLength(0) assertion, add two more assertions to verify that mainMockState.archiveCoverRunCalls and mainMockState.archiveChapterRunCalls also have length 0, ensuring that accidental misrouting in non-maintenance paths is properly caught.
🤖 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 `@data/help/commands/maintenance/chapter/reset.jinja`:
- Line 18: The help text on line 18 of the reset.jinja file incorrectly
references the `--stage` flag, but the actual command usage shown on line 10
uses `--to <stage>` instead. Update the wording in the line that begins with "-
`--stage summary --confirm` is not supported" to replace `--stage` with `--to`
so the flag reference matches the correct command syntax documented in the usage
section.
In `@data/help/commands/maintenance/chapter/set-summary.jinja`:
- Line 15: The workflow example at line 15 uses an incomplete command `read >
/tmp/summary.md` which is ambiguous and doesn't show the full CLI command
structure used elsewhere in the documentation. Replace this ambiguous example
with a complete, explicitly runnable workflow that demonstrates the full command
shape, showing how to use the CLI command to read into a file, edit it, and then
pass it back with the `--input` option in the proper command format.
In `@data/help/commands/transform.jinja`:
- Line 8: The spinedigest transform command synopsis is missing the standard
help flag documentation. Add --help|-h to the command synopsis in the transform
usage line to maintain consistency with the actual CLI capabilities and keep the
documentation complete. This flag should be included along with the other
optional flags to reflect that the help option is supported by the command.
In `@docs/zh-CN/cli.md`:
- Line 106: The chapter command synopsis in the Chinese CLI reference
documentation at docs/zh-CN/cli.md is missing the set-title subcommand from its
list of supported operations. Update the spinedigest chapter command synopsis on
line 106 to include set-title in the list of available subcommands alongside
list, status, add, remove, reset, set-source, and set-summary to make this
shipped action discoverable in the documentation.
In `@src/facade/archive-view.ts`:
- Around line 924-941: The fallback to fragment-based grouping only occurs when
snakes.length is zero, but does not account for the case where snake records
exist but all resulting groups are filtered out at the filter call. Modify the
return statement to capture the filtered result from the Promise.all chain and
check if it is empty. If the filtered array has zero length, return the fallback
result from listChapterNodeGroupsByFragment instead. Otherwise, return the
non-empty filtered array. This ensures that when all snake node groups resolve
to empty nodeCount values, the function properly falls back to fragment-based
grouping rather than returning an empty array.
- Around line 363-375: The code currently only includes the meta:book item when
meta is defined, creating an inconsistency with listArchiveObjects("meta") which
exposes meta entries regardless. Remove the `if (meta !== undefined)` condition
that gates the items.push() call, so that meta:book is always exposed when types
includes "meta". Instead of conditionally pushing, modify the snippet field to
conditionally use either formatMetaSummary(meta) when meta is defined or a
placeholder string (such as "No metadata available") when meta is undefined,
ensuring the item is always added to the collection.
In `@test/cli/archive.test.ts`:
- Around line 243-255: The grepArchiveObjects mock in the test file has
semantics that do not match the actual grep facade contract. Change the match
property from "any" to "all" and update the terms array to reflect how grep
actually normalizes phrase terms (rather than splitting them into individual
terms). This ensures the mock accurately represents the real grep facade
behavior and prevents hiding regressions in grep-specific CLI messaging paths.
---
Outside diff comments:
In `@src/cli/args.ts`:
- Around line 435-439: The parser branches are not rejecting all globally-known
options, allowing flags like --id, --cursor, --type, --order, --match,
--chapter, --to to be silently ignored instead of failing fast. In
src/cli/args.ts at lines 435-439, add rejectConvertFlag calls for all missing
globally-known flags (--id, --cursor, --type, --order, --match, --chapter, --to)
in addition to the existing rejectConvertFlag calls for budget, confirm, and
json. Apply the same set of rejectConvertFlag calls to the other affected sites
at lines 1146-1152 and 1632-1641 to ensure consistent validation across all
command parser branches.
- Around line 2093-2114: Remove the stale `"fragments"` string comparison from
the isArchiveAction type guard function. The `"fragments"` action is not a valid
member of the CLIArchiveAction type and has no corresponding handler in
parseArchiveArguments, so accepting it as valid causes invalid input to route
into an internal parser failure instead of producing a clean unknown-command
error. Simply delete the line checking `value === "fragments" ||` from the
function's conditional chain.
---
Nitpick comments:
In `@test/cli/main.test.ts`:
- Line 165: Strengthen the negative-dispatch assertions at all four locations
(lines 165, 182, 208, and 251) by adding checks for archiveCoverRunCalls and
archiveChapterRunCalls in addition to the existing archiveMetaRunCalls checks.
For each location, after the existing
expect(mainMockState.archiveMetaRunCalls).toHaveLength(0) assertion, add two
more assertions to verify that mainMockState.archiveCoverRunCalls and
mainMockState.archiveChapterRunCalls also have length 0, ensuring that
accidental misrouting in non-maintenance paths is properly caught.
🪄 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: 379c2d35-2230-402d-a456-70114a39dc15
📒 Files selected for processing (93)
README.mdREADME_zh-CN.mddata/help/commands/archive/evidence.jinjadata/help/commands/archive/find.jinjadata/help/commands/archive/grep.jinjadata/help/commands/archive/list.jinjadata/help/commands/archive/ls.jinjadata/help/commands/archive/pack.jinjadata/help/commands/archive/page.jinjadata/help/commands/archive/read.jinjadata/help/commands/archive/status.jinjadata/help/commands/config-status.jinjadata/help/commands/maintenance/chapter.jinjadata/help/commands/maintenance/chapter/add.jinjadata/help/commands/maintenance/chapter/list.jinjadata/help/commands/maintenance/chapter/remove.jinjadata/help/commands/maintenance/chapter/reset.jinjadata/help/commands/maintenance/chapter/set-source.jinjadata/help/commands/maintenance/chapter/set-summary.jinjadata/help/commands/maintenance/chapter/set-title.jinjadata/help/commands/maintenance/chapter/status.jinjadata/help/commands/maintenance/cover.jinjadata/help/commands/maintenance/meta.jinjadata/help/commands/root.jinjadata/help/commands/sdpub/cat.jinjadata/help/commands/sdpub/chapter.jinjadata/help/commands/sdpub/chapter/reset.jinjadata/help/commands/sdpub/chapter/set-summary.jinjadata/help/commands/sdpub/graph.jinjadata/help/commands/sdpub/graph/blame.jinjadata/help/commands/sdpub/graph/grep.jinjadata/help/commands/sdpub/graph/log.jinjadata/help/commands/sdpub/graph/neighbors.jinjadata/help/commands/sdpub/graph/path.jinjadata/help/commands/sdpub/graph/show.jinjadata/help/commands/sdpub/graph/status.jinjadata/help/commands/sdpub/index.jinjadata/help/commands/sdpub/info.jinjadata/help/commands/sdpub/list.jinjadata/help/commands/sdpub/stage.jinjadata/help/commands/sdpub/stage/advance.jinjadata/help/commands/sdpub/stage/pending.jinjadata/help/commands/sdpub/toc.jinjadata/help/commands/transform.jinjadata/help/topics/ai.jinjadata/help/topics/command.jinjadata/help/topics/config-file.jinjadata/help/topics/config.jinjadata/help/topics/env.jinjadata/help/topics/format.jinjadata/help/topics/index.jinjadata/help/topics/overview.jinjadata/help/topics/recipe.jinjadata/help/topics/runtime.jinjadata/help/topics/sdpub.jinjadata/help/topics/task.jinjadata/help/topics/troubleshoot.jinjadocs/en/ai-agents.mddocs/en/cli.mddocs/en/quickstart.mddocs/sdpub.mddocs/zh-CN/ai-agents.mddocs/zh-CN/cli.mddocs/zh-CN/quickstart.mdsrc/cli/archive-chapter.tssrc/cli/archive-maintenance.tssrc/cli/archive.tssrc/cli/args.tssrc/cli/errors.tssrc/cli/help.tssrc/cli/main.tssrc/cli/sdpub-graph.tssrc/cli/sdpub-stage.tssrc/cli/sdpub.tssrc/facade/archive-view.tssrc/facade/chapter.tssrc/facade/graph.tssrc/facade/index.tssrc/facade/spine-digest.tssrc/output/epub/book.tssrc/output/plain-text.tssrc/serial.tstest/cli/README.mdtest/cli/archive-chapter.test.tstest/cli/archive-maintenance.test.tstest/cli/archive.test.tstest/cli/args.test.tstest/cli/main.test.tstest/cli/sdpub-graph.test.tstest/cli/sdpub-stage.test.tstest/cli/sdpub.test.tstest/facade/archive-view.test.tstest/facade/chapter.test.ts
💤 Files with no reviewable changes (29)
- data/help/commands/sdpub/stage/pending.jinja
- data/help/commands/archive/evidence.jinja
- data/help/topics/sdpub.jinja
- data/help/commands/sdpub/chapter.jinja
- data/help/commands/sdpub/toc.jinja
- data/help/commands/sdpub/info.jinja
- docs/en/quickstart.md
- data/help/commands/sdpub/stage/advance.jinja
- data/help/commands/sdpub/cat.jinja
- data/help/commands/sdpub/graph/grep.jinja
- src/cli/sdpub-stage.ts
- data/help/commands/sdpub/index.jinja
- test/cli/sdpub-graph.test.ts
- data/help/commands/sdpub/graph/log.jinja
- data/help/commands/sdpub/chapter/set-summary.jinja
- data/help/commands/sdpub/graph.jinja
- data/help/commands/sdpub/chapter/reset.jinja
- data/help/commands/sdpub/graph/show.jinja
- data/help/commands/sdpub/stage.jinja
- data/help/commands/sdpub/list.jinja
- data/help/commands/sdpub/graph/path.jinja
- data/help/commands/sdpub/graph/neighbors.jinja
- src/cli/sdpub.ts
- docs/zh-CN/quickstart.md
- data/help/commands/sdpub/graph/status.jinja
- test/cli/sdpub-stage.test.ts
- test/cli/sdpub.test.ts
- src/cli/sdpub-graph.ts
- data/help/commands/sdpub/graph/blame.jinja
| graphed Keep source text and graph data; delete summary. | ||
|
|
||
| Behavior: | ||
| - `--stage summary --confirm` is not supported because reset is a backward operation. |
There was a problem hiding this comment.
Fix reset flag wording to match command usage.
Line 18 references --stage, but this command’s usage on Line 10 is --to <stage>. The current wording mixes reset and build flag semantics.
Suggested patch
- - `--stage summary --confirm` is not supported because reset is a backward operation.
+ - `--to summary` is not supported because reset is a backward operation.
+ - To move forward to summary, use `spinedigest build <path> --chapter <id> --stage summary --confirm`.📝 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
| - `--stage summary --confirm` is not supported because reset is a backward operation. | |
| - `--to summary` is not supported because reset is a backward operation. | |
| - To move forward to summary, use `spinedigest build <path> --chapter <id> --stage summary --confirm`. |
🤖 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 `@data/help/commands/maintenance/chapter/reset.jinja` at line 18, The help text
on line 18 of the reset.jinja file incorrectly references the `--stage` flag,
but the actual command usage shown on line 10 uses `--to <stage>` instead.
Update the wording in the line that begins with "- `--stage summary --confirm`
is not supported" to replace `--stage` with `--to` so the flag reference matches
the correct command syntax documented in the usage section.
| Input: | ||
| - If `--input` is omitted, summary text is read from stdin. | ||
| - Summary text is stored as the chapter's final summary. | ||
| - A common workflow is `read > /tmp/summary.md`, edit the file, then pass it back with `--input`. |
There was a problem hiding this comment.
Make the workflow example explicitly runnable.
Line 15 uses read > /tmp/summary.md, which is ambiguous in shell context and omits the CLI command shape used elsewhere in this page.
Suggested patch
- - A common workflow is `read > /tmp/summary.md`, edit the file, then pass it back with `--input`.
+ - A common workflow is `spinedigest read <path> ... > /tmp/summary.md`, edit the file, then pass it back with `--input`.📝 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
| - A common workflow is `read > /tmp/summary.md`, edit the file, then pass it back with `--input`. | |
| - A common workflow is `spinedigest read <path> ... > /tmp/summary.md`, edit the file, then pass it back with `--input`. |
🤖 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 `@data/help/commands/maintenance/chapter/set-summary.jinja` at line 15, The
workflow example at line 15 uses an incomplete command `read > /tmp/summary.md`
which is ambiguous and doesn't show the full CLI command structure used
elsewhere in the documentation. Replace this ambiguous example with a complete,
explicitly runnable workflow that demonstrates the full command shape, showing
how to use the CLI command to read into a file, edit it, and then pass it back
with the `--input` option in the proper command format.
| Run a direct one-shot digest/export without creating a reusable `.sdpub` archive. | ||
|
|
||
| Usage: | ||
| spinedigest transform [--input <path>] [--output <path>] [--input-format <format>] [--output-format <format>] [--digest-dir <path>] [--llm <json>] [--prompt <text>] [--stage <planned|sourced|graphed|summarized>] [--verbose|-v] |
There was a problem hiding this comment.
Include --help|-h in the transform usage synopsis.
Line 8 omits the standard help flag even though spinedigest transform --help is supported. Keeping the synopsis complete avoids CLI-doc drift.
Proposed patch
- spinedigest transform [--input <path>] [--output <path>] [--input-format <format>] [--output-format <format>] [--digest-dir <path>] [--llm <json>] [--prompt <text>] [--stage <planned|sourced|graphed|summarized>] [--verbose|-v]
+ spinedigest transform [--input <path>] [--output <path>] [--input-format <format>] [--output-format <format>] [--digest-dir <path>] [--llm <json>] [--prompt <text>] [--stage <planned|sourced|graphed|summarized>] [--verbose|-v] [--help|-h]📝 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
| spinedigest transform [--input <path>] [--output <path>] [--input-format <format>] [--output-format <format>] [--digest-dir <path>] [--llm <json>] [--prompt <text>] [--stage <planned|sourced|graphed|summarized>] [--verbose|-v] | |
| spinedigest transform [--input <path>] [--output <path>] [--input-format <format>] [--output-format <format>] [--digest-dir <path>] [--llm <json>] [--prompt <text>] [--stage <planned|sourced|graphed|summarized>] [--verbose|-v] [--help|-h] |
🤖 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 `@data/help/commands/transform.jinja` at line 8, The spinedigest transform
command synopsis is missing the standard help flag documentation. Add --help|-h
to the command synopsis in the transform usage line to maintain consistency with
the actual CLI capabilities and keep the documentation complete. This flag
should be included along with the other optional flags to reflect that the help
option is supported by the command.
| spinedigest sdpub graph <status|log|show|grep|neighbors|blame|path> <path> --chapter <id> [options] | ||
| spinedigest meta <archive.sdpub> [metadata options] [--json] | ||
| spinedigest cover <archive.sdpub> | ||
| spinedigest chapter <list|status|add|remove|reset|set-source|set-summary> <path> [options] |
There was a problem hiding this comment.
Add set-title to the chapter command synopsis.
The Chinese CLI reference omits set-title from the supported chapter subcommands, but the parser/help surface supports it. This makes a shipped action undiscoverable in this page.
Suggested doc fix
-spinedigest chapter <list|status|add|remove|reset|set-source|set-summary> <path> [options]
+spinedigest chapter <list|status|add|remove|reset|set-source|set-summary|set-title> <path> [options]📝 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
| spinedigest chapter <list|status|add|remove|reset|set-source|set-summary> <path> [options] | |
| spinedigest chapter <list|status|add|remove|reset|set-source|set-summary|set-title> <path> [options] |
🤖 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 `@docs/zh-CN/cli.md` at line 106, The chapter command synopsis in the Chinese
CLI reference documentation at docs/zh-CN/cli.md is missing the set-title
subcommand from its list of supported operations. Update the spinedigest chapter
command synopsis on line 106 to include set-title in the list of available
subcommands alongside list, status, add, remove, reset, set-source, and
set-summary to make this shipped action discoverable in the documentation.
Comment on lines
+363
to
+375
| if (types.includes("meta")) { | ||
| const meta = await document.readBookMeta(); | ||
|
|
||
| if (meta !== undefined) { | ||
| items.push({ | ||
| field: "metadata", | ||
| id: "meta:book", | ||
| snippet: formatMetaSummary(meta), | ||
| title: meta.title ?? "Book metadata", | ||
| type: "meta", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Always expose meta:book in collection listings
Line 366 currently suppresses the meta object when metadata is missing. That makes collection-based discovery inconsistent with listArchiveObjects("meta"), which still exposes a meta entry. Keep meta:book visible and use a missing placeholder snippet instead.
Proposed fix
if (types.includes("meta")) {
const meta = await document.readBookMeta();
- if (meta !== undefined) {
- items.push({
- field: "metadata",
- id: "meta:book",
- snippet: formatMetaSummary(meta),
- title: meta.title ?? "Book metadata",
- type: "meta",
- });
- }
+ items.push({
+ field: "metadata",
+ id: "meta:book",
+ snippet: formatMetaSummary(meta),
+ title: meta?.title ?? "Book metadata",
+ type: "meta",
+ });
}🤖 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/facade/archive-view.ts` around lines 363 - 375, The code currently only
includes the meta:book item when meta is defined, creating an inconsistency with
listArchiveObjects("meta") which exposes meta entries regardless. Remove the `if
(meta !== undefined)` condition that gates the items.push() call, so that
meta:book is always exposed when types includes "meta". Instead of conditionally
pushing, modify the snippet field to conditionally use either
formatMetaSummary(meta) when meta is defined or a placeholder string (such as
"No metadata available") when meta is undefined, ensuring the item is always
added to the collection.
Comment on lines
+924
to
+941
| if (snakes.length === 0) { | ||
| return listChapterNodeGroupsByFragment(document, chapterId); | ||
| } | ||
|
|
||
| return ( | ||
| await Promise.all( | ||
| snakes.map(async (snake) => | ||
| createArchiveNodeGroup({ | ||
| groupId: snake.localSnakeId, | ||
| id: `node-group:${chapterId}:${snake.localSnakeId}`, | ||
| nodes: await listSnakeNodeLabels(document, snake), | ||
| weight: snake.weight, | ||
| wordsCount: snake.wordsCount, | ||
| }), | ||
| ), | ||
| ) | ||
| ).filter((group) => group.nodeCount > 0); | ||
| } |
There was a problem hiding this comment.
Fallback grouping is missing when snake groups resolve to empty
Line 924 only falls back when there are zero snake records. If snake records exist but all are filtered out at Line 940 (e.g., stale snake-chunk links), chapter pages return empty nodeGroups despite non-zero nodeCount. Add a post-filter fallback to fragment-based grouping.
Proposed fix
async function listChapterNodeGroups(
document: Document,
chapterId: number,
): Promise<readonly ArchiveNodeGroup[]> {
const snakes = await document.snakes.listBySerial(chapterId);
if (snakes.length === 0) {
return listChapterNodeGroupsByFragment(document, chapterId);
}
- return (
+ const groups = (
await Promise.all(
snakes.map(async (snake) =>
createArchiveNodeGroup({
groupId: snake.localSnakeId,
id: `node-group:${chapterId}:${snake.localSnakeId}`,
nodes: await listSnakeNodeLabels(document, snake),
weight: snake.weight,
wordsCount: snake.wordsCount,
}),
),
)
).filter((group) => group.nodeCount > 0);
+
+ return groups.length > 0
+ ? groups
+ : await listChapterNodeGroupsByFragment(document, chapterId);
}📝 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
| if (snakes.length === 0) { | |
| return listChapterNodeGroupsByFragment(document, chapterId); | |
| } | |
| return ( | |
| await Promise.all( | |
| snakes.map(async (snake) => | |
| createArchiveNodeGroup({ | |
| groupId: snake.localSnakeId, | |
| id: `node-group:${chapterId}:${snake.localSnakeId}`, | |
| nodes: await listSnakeNodeLabels(document, snake), | |
| weight: snake.weight, | |
| wordsCount: snake.wordsCount, | |
| }), | |
| ), | |
| ) | |
| ).filter((group) => group.nodeCount > 0); | |
| } | |
| if (snakes.length === 0) { | |
| return listChapterNodeGroupsByFragment(document, chapterId); | |
| } | |
| const groups = ( | |
| await Promise.all( | |
| snakes.map(async (snake) => | |
| createArchiveNodeGroup({ | |
| groupId: snake.localSnakeId, | |
| id: `node-group:${chapterId}:${snake.localSnakeId}`, | |
| nodes: await listSnakeNodeLabels(document, snake), | |
| weight: snake.weight, | |
| wordsCount: snake.wordsCount, | |
| }), | |
| ), | |
| ) | |
| ).filter((group) => group.nodeCount > 0); | |
| return groups.length > 0 | |
| ? groups | |
| : await listChapterNodeGroupsByFragment(document, chapterId); | |
| } |
🤖 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/facade/archive-view.ts` around lines 924 - 941, The fallback to
fragment-based grouping only occurs when snakes.length is zero, but does not
account for the case where snake records exist but all resulting groups are
filtered out at the filter call. Modify the return statement to capture the
filtered result from the Promise.all chain and check if it is empty. If the
filtered array has zero length, return the fallback result from
listChapterNodeGroupsByFragment instead. Otherwise, return the non-empty
filtered array. This ensures that when all snake node groups resolve to empty
nodeCount values, the function properly falls back to fragment-based grouping
rather than returning an empty array.
Comment on lines
+243
to
+255
| grepArchiveObjects: vi.fn(() => | ||
| Promise.resolve({ | ||
| chapters: null, | ||
| items: archiveMockState.grepHits, | ||
| lens: "exact", | ||
| lensHint: null, | ||
| limit: 20, | ||
| match: "any", | ||
| nextCursor: null, | ||
| order: "doc-asc", | ||
| query: "exact phrase", | ||
| terms: ["exact", "phrase"], | ||
| types: null, |
There was a problem hiding this comment.
Align grep mock semantics with actual facade contract.
Line 250 sets match: "any" and Lines 254-255 split terms, but grep currently returns exact phrase semantics (match: "all" with normalized phrase terms). This mismatch can hide regressions in grep-specific CLI messaging paths.
Suggested fix
- grepArchiveObjects: vi.fn(() =>
+ grepArchiveObjects: vi.fn((_document: unknown, query: string) =>
Promise.resolve({
chapters: null,
items: archiveMockState.grepHits,
lens: "exact",
lensHint: null,
limit: 20,
- match: "any",
+ match: "all",
nextCursor: null,
order: "doc-asc",
- query: "exact phrase",
- terms: ["exact", "phrase"],
+ query,
+ terms: [query.trim().toLowerCase()],
types: null,
}),
),📝 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
| grepArchiveObjects: vi.fn(() => | |
| Promise.resolve({ | |
| chapters: null, | |
| items: archiveMockState.grepHits, | |
| lens: "exact", | |
| lensHint: null, | |
| limit: 20, | |
| match: "any", | |
| nextCursor: null, | |
| order: "doc-asc", | |
| query: "exact phrase", | |
| terms: ["exact", "phrase"], | |
| types: null, | |
| grepArchiveObjects: vi.fn((_document: unknown, query: string) => | |
| Promise.resolve({ | |
| chapters: null, | |
| items: archiveMockState.grepHits, | |
| lens: "exact", | |
| lensHint: null, | |
| limit: 20, | |
| match: "all", | |
| nextCursor: null, | |
| order: "doc-asc", | |
| query, | |
| terms: [query.trim().toLowerCase()], | |
| types: null, |
🤖 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 `@test/cli/archive.test.ts` around lines 243 - 255, The grepArchiveObjects mock
in the test file has semantics that do not match the actual grep facade
contract. Change the match property from "any" to "all" and update the terms
array to reflect how grep actually normalizes phrase terms (rather than
splitting them into individual terms). This ensures the mock accurately
represents the real grep facade behavior and prevents hiding regressions in
grep-specific CLI messaging paths.
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
.sdpubarchives.sdpubcommand family and evidence/sentence-facing surfaces from routine CLI usage.findsemantics for agent workflows, including any-match defaults, search lenses, paging, and typed discovery hints.Verification
pnpm run lint:fixpnpm verifypnpm test:runpnpm exec tsc -p tsconfig.json --noEmit