Add a staged implementation plan to address gaps from the initial implementation of the scene chunking module
This commit is contained in:
@@ -1,8 +1,7 @@
|
||||
# Chunk Module Implementation Plan
|
||||
# Chunk Follow-Up Implementation Plan
|
||||
|
||||
This plan implements the accepted target state in
|
||||
[Chunk Module Roadmap](chunk.md). It is written for an LLM coding agent that
|
||||
will implement each stage in order.
|
||||
This plan addresses review findings from the first `dnd/scenes` implementation.
|
||||
It is written for an LLM coding agent that will implement each stage in order.
|
||||
|
||||
Before beginning any stage, review:
|
||||
|
||||
@@ -14,278 +13,213 @@ Before beginning any stage, review:
|
||||
Do not move planned behavior into non-roadmap docs until the corresponding code
|
||||
is implemented. Do not revert unrelated user changes.
|
||||
|
||||
## Stage 1: Framework Chunk Contract
|
||||
## Goals
|
||||
|
||||
Goal: make LLM-backed chunking first-class and enforce generic chunk result
|
||||
invariants without adding any D&D-specific framework behavior.
|
||||
- Record chunker prompt and response-schema provenance in run manifests.
|
||||
- Ensure downstream extractors receive canonical source units from the source
|
||||
document, not chunker-mutated unit payloads.
|
||||
- Prevent empty or whitespace-only scene caveats from becoming warnings.
|
||||
|
||||
## Stage 1: Run-Manifest Module Metadata
|
||||
|
||||
Goal: make non-lane module provenance auditable without adding
|
||||
chunker-specific fields or D&D-specific framework behavior.
|
||||
|
||||
Design decision:
|
||||
|
||||
- Add a generic top-level run manifest metadata map for singleton pipeline
|
||||
modules:
|
||||
|
||||
```go
|
||||
ModuleMetadata map[string]map[string]any `json:"module_metadata,omitempty"`
|
||||
```
|
||||
|
||||
- Use stable stage keys:
|
||||
- `input`
|
||||
- `chunker`
|
||||
- `output`
|
||||
- Keep existing artifact lane metadata under `ArtifactLaneManifest.Metadata`.
|
||||
Do not move extractor, merger, normalizer, or validator metadata into the
|
||||
top-level map in this stage.
|
||||
- Record metadata only when a built module implements
|
||||
`contracts.ManifestMetadataProvider` and returns non-empty metadata.
|
||||
- Continue to reject raw prompts, raw response schemas, source text, provider
|
||||
payloads, and secrets from manifest metadata by convention and tests.
|
||||
|
||||
Code changes:
|
||||
|
||||
- Add `LLMClient contracts.StructuredLLMClient` to `contracts.ChunkRequest` in
|
||||
`internal/framework/contracts/contracts.go`.
|
||||
- Update `internal/framework/pipeline/runner.go` so the runner passes
|
||||
`input.LLMClient` to the chunker in `contracts.ChunkRequest`.
|
||||
- Add framework-level chunk result validation after `chunker.Chunk` returns and
|
||||
before lanes execute.
|
||||
- Keep validation source-generic. The validator should reject:
|
||||
- empty chunk ID;
|
||||
- duplicate chunk ID;
|
||||
- chunk `SourceID` that does not match the source document ID;
|
||||
- chunk `Index` that does not match returned order;
|
||||
- empty chunk units;
|
||||
- repeated source unit inside one chunk;
|
||||
- source unit not found in the source document;
|
||||
- chunk units that do not appear in source-document order.
|
||||
- The validator must not require complete coverage and must not reject overlap
|
||||
between different chunks.
|
||||
- Preserve existing warning behavior: append chunker warnings before returning
|
||||
chunk errors, as the runner does today.
|
||||
|
||||
Documentation changes:
|
||||
|
||||
- Update implemented internal docs under `docs/internal/` to define the chunk
|
||||
module API and validation invariants once the code exists.
|
||||
- Keep examples and user docs unchanged in this stage unless an existing doc
|
||||
becomes inaccurate.
|
||||
- Add `ModuleMetadata` to `internal/core/artifacts.RunManifest`.
|
||||
- Add a small helper in `internal/framework/pipeline/runner.go` to attach
|
||||
top-level module metadata by stage key.
|
||||
- After building the input adapter, chunker, and output encoder, call that
|
||||
helper with keys `input`, `chunker`, and `output` respectively.
|
||||
- Keep `setLaneManifestMetadata` for lane-owned modules. If practical, share
|
||||
metadata cloning logic with the new helper.
|
||||
- Ensure failed runs that already have a manifest also retain any metadata
|
||||
collected before the failure.
|
||||
|
||||
Tests:
|
||||
|
||||
- Update contract tests for the new `ChunkRequest.LLMClient` field where useful.
|
||||
- Add focused pipeline runner tests for each invalid chunk result case listed
|
||||
above.
|
||||
- Add runner tests proving partial coverage and overlapping chunks remain
|
||||
accepted.
|
||||
- Run:
|
||||
- Add pipeline runner tests proving top-level metadata is recorded for a fake
|
||||
chunker that implements `ManifestMetadataProvider`.
|
||||
- Add a test proving the existing lane metadata behavior remains unchanged.
|
||||
- Add a CLI or output integration test proving a `dnd/scenes` run manifest
|
||||
contains `module_metadata.chunker` with prompt/schema provenance.
|
||||
- Add a negative assertion that raw prompt text, raw schema JSON, source text,
|
||||
provider payloads, and API key-like fields are not present in the scene
|
||||
chunker metadata.
|
||||
|
||||
Documentation:
|
||||
|
||||
- Update `docs/internal/pipeline.md` to describe top-level metadata for input,
|
||||
chunker, and output modules, and lane metadata for lane modules.
|
||||
- Update `docs/internal/modules.md` to say that `dnd/scenes` prompt/schema
|
||||
provenance appears under `module_metadata.chunker`.
|
||||
- Update `docs/integrations/json-output.md` and `docs/operations.md` if their
|
||||
manifest descriptions need to mention `module_metadata`.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/framework/contracts ./internal/framework/pipeline
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- Existing generic chunking still works.
|
||||
- A fake chunker can receive the structured LLM client through `ChunkRequest`.
|
||||
- Framework tests prove the accepted generic chunk invariants.
|
||||
|
||||
## Stage 2: D&D Scene Assets And Module Skeleton
|
||||
|
||||
Goal: revise the draft D&D scene prompt and schema into module-owned assets and
|
||||
add load/render plumbing without registering production behavior.
|
||||
|
||||
Asset decisions:
|
||||
|
||||
- Rename `internal/modules/chunk/dnd/scenes/assets/schemas/scene_map.schema.json`
|
||||
to `internal/modules/chunk/dnd/scenes/assets/schemas/dnd_scenes.v1.json`.
|
||||
- Use these schema constants unless a code-local naming conflict requires a
|
||||
mechanical adjustment:
|
||||
- prompt ID: `dnd.scenes`
|
||||
- response schema key: `dnd_scenes`
|
||||
- response schema ID: `notarius.dnd.scenes`
|
||||
- response schema version: `v1`
|
||||
- response schema name: `notarius_dnd_scenes_v1`
|
||||
- Use this structured response shape:
|
||||
|
||||
```json
|
||||
{
|
||||
"scenes": [
|
||||
{
|
||||
"start_unit_id": "seg-001",
|
||||
"end_unit_id": "seg-010",
|
||||
"short_title": "Ambush at the gate",
|
||||
"primary_mode": "Combat",
|
||||
"main_participants": ["Aria", "Bandit mage"],
|
||||
"summary": "The party fights the bandit mage at the gate.",
|
||||
"boundary_note": "The scene begins when combat starts and ends when the immediate threat is resolved.",
|
||||
"boundary_confidence": "High"
|
||||
}
|
||||
],
|
||||
"boundary_caveats": []
|
||||
}
|
||||
```
|
||||
|
||||
- Required top-level fields: `scenes`, `boundary_caveats`.
|
||||
- Required scene fields: `start_unit_id`, `end_unit_id`, `short_title`,
|
||||
`primary_mode`, `main_participants`, `summary`, `boundary_note`,
|
||||
`boundary_confidence`.
|
||||
- Boundary fields are source-unit ID strings, not integers.
|
||||
- `primary_mode` enum: `Recap`, `Discussion`, `Combat`, `Narrative`.
|
||||
- `boundary_confidence` enum: `High`, `Medium`, `Low`.
|
||||
- Keep `additionalProperties: false` throughout the schema.
|
||||
- Do not include model-authored final chunk IDs or chunk indexes in the schema.
|
||||
The Go module assigns deterministic chunk IDs and indexes.
|
||||
|
||||
Prompt decisions:
|
||||
|
||||
- Keep D&D-specific scene guidance in the D&D scene module.
|
||||
- Make the user prompt a Go template similar to the spell extractor prompt.
|
||||
- Include source document ID and ordered source units.
|
||||
- Include selected source-unit metadata when present: `speaker`, `start`, and
|
||||
`end`.
|
||||
- Align prompt terms exactly with schema field names and enum values.
|
||||
- Keep `dnd/scenes` module policy explicit in the prompt: full coverage,
|
||||
sequential scenes, no gaps, no overlap, exact source-unit IDs.
|
||||
|
||||
Code changes:
|
||||
|
||||
- Add `assets.go` with an `embed.FS` for prompts and schemas.
|
||||
- Add `schema.go` with the constants and a `loadResponseSchema` function using
|
||||
`llm.LoadResponseSchema`, following the pattern in
|
||||
`internal/modules/extract/dnd/spells/schema.go`.
|
||||
- Add prompt rendering code using `framework/prompt.Bundle`, following the
|
||||
pattern in `internal/modules/extract/dnd/spells/prompt.go`.
|
||||
- Add internal response structs for the schema shape.
|
||||
- Do not register the module in `internal/cli/catalog.go` in this stage.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add tests that the schema loads, is valid JSON, has the expected metadata, and
|
||||
rejects the old integer-boundary assumption through Go-side type expectations.
|
||||
- Add prompt rendering tests that source unit IDs and selected metadata appear
|
||||
in the rendered user prompt.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/modules/chunk/dnd/scenes
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- The scene schema and prompts are loadable embedded assets.
|
||||
- The prompt/schema terminology is internally consistent.
|
||||
- No production catalog behavior changes yet.
|
||||
|
||||
## Stage 3: D&D Scene Chunker Implementation
|
||||
|
||||
Goal: implement `dnd/scenes` as a contract-compliant chunk module with strict
|
||||
module-owned validation.
|
||||
|
||||
Module decisions:
|
||||
|
||||
- Package path: `internal/modules/chunk/dnd/scenes`.
|
||||
- Package name: `scenes`.
|
||||
- Module key: `dnd/scenes`.
|
||||
- `ModuleSpec`:
|
||||
- `Stage`: `pipeline.StageChunk`
|
||||
- `Requires`: `source.transcript`
|
||||
- `Provides`: `chunks`, `chunks.scenes`
|
||||
- Constructor: `New() *Chunker`.
|
||||
- Registration function: `Register(registry *pipeline.ChunkerRegistry) error`.
|
||||
- No module options initially. Reject non-empty options with an actionable
|
||||
module-prefixed error unless a clear option is implemented in the same stage.
|
||||
|
||||
Chunking behavior:
|
||||
|
||||
- Validate `context.Context`, source document, non-empty source units, and
|
||||
non-nil `LLMClient`.
|
||||
- Render the scene prompt over the full source document.
|
||||
- Call `LLMClient.CompleteStructured` with:
|
||||
- `StageName`: `dnd/scenes`
|
||||
- response schema name and schema JSON from the module schema loader.
|
||||
- Validate the decoded response before producing chunks:
|
||||
- `scenes` must be present and non-empty;
|
||||
- every boundary ID must exist in the source document;
|
||||
- each scene start must be at or before its end;
|
||||
- the first scene starts at the first source unit;
|
||||
- the final scene ends at the final source unit;
|
||||
- scenes are contiguous in source order;
|
||||
- scenes do not overlap;
|
||||
- required metadata fields are non-empty after trimming;
|
||||
- `main_participants` entries are trimmed and empty entries rejected.
|
||||
- Assign deterministic chunk fields:
|
||||
- `ID`: `scene-000001`, `scene-000002`, and so on;
|
||||
- `SourceID`: source document ID;
|
||||
- `Index`: zero-based returned order;
|
||||
- `Units`: defensive copies of the source units in the scene range.
|
||||
- Store per-scene metadata on each chunk:
|
||||
- `scene_title`
|
||||
- `primary_mode`
|
||||
- `main_participants`
|
||||
- `summary`
|
||||
- `boundary_note`
|
||||
- `boundary_confidence`
|
||||
- `start_unit_id`
|
||||
- `end_unit_id`
|
||||
- `unit_count`
|
||||
- Convert each `boundary_caveats` entry into a `contracts.Warning` with:
|
||||
- `Scope`: `dnd/scenes`
|
||||
- `ReasonCode`: `scene_boundary_caveat`
|
||||
- `Message`: the caveat text.
|
||||
- Fail explicitly for malformed model output. Do not fall back to `generic`.
|
||||
- Implement `contracts.ManifestMetadataProvider` and include prompt and
|
||||
response-schema provenance without raw prompts, raw schemas, source text, or
|
||||
secrets.
|
||||
|
||||
Tests:
|
||||
|
||||
- Registration and `ModuleSpec`.
|
||||
- Successful chunking from a fake LLM response.
|
||||
- Prompt request uses the expected schema name and schema JSON.
|
||||
- Caveats become warnings.
|
||||
- Defensive copy behavior for source units and metadata.
|
||||
- Errors for nil context, nil source, invalid source, nil LLM client, empty
|
||||
model scenes, unknown boundary ID, out-of-order boundaries, gaps, overlap,
|
||||
incomplete coverage, empty metadata fields, and non-empty unsupported options.
|
||||
- Manifest metadata contains prompt/schema provenance.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/modules/chunk/dnd/scenes
|
||||
go test ./internal/core/artifacts
|
||||
go test ./internal/framework/pipeline
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- `dnd/scenes` works in focused tests with fake LLM clients.
|
||||
- It is still not production-registered unless Stage 4 is completed.
|
||||
|
||||
## Stage 4: Production Registration And Implemented Docs
|
||||
|
||||
Goal: make `dnd/scenes` available in production configuration and document only
|
||||
the behavior that now exists.
|
||||
|
||||
Code changes:
|
||||
|
||||
- Register `dnd/scenes` in `internal/cli/catalog.go`.
|
||||
- Add or update catalog/default module tests so the production catalog exposes
|
||||
the new chunk module.
|
||||
- Add CLI/config validation tests proving a pipeline can select
|
||||
`chunk: dnd/scenes`.
|
||||
- Do not change the existing maintained example config unless the related CLI
|
||||
fixture tests are updated to keep it loadable and useful.
|
||||
|
||||
Documentation changes:
|
||||
|
||||
- Update `docs/config.md` implemented production module tables and chunk module
|
||||
notes.
|
||||
- Update `docs/cli.md` implemented production module list.
|
||||
- Update `docs/internal/modules.md` with `dnd/scenes` behavior, capabilities,
|
||||
metadata, and failure policy.
|
||||
- Update or add internal chunk-module documentation if Stage 1 did not already
|
||||
create a clear API reference.
|
||||
- Update `docs/troubleshooting.md` for common scene chunker failures:
|
||||
malformed model output, invalid boundaries, incomplete coverage, and provider
|
||||
failures during chunking.
|
||||
- Keep roadmap docs for any deferred options or future prompt tuning.
|
||||
|
||||
Tests:
|
||||
|
||||
```sh
|
||||
go test ./internal/cli
|
||||
go test ./internal/core/config
|
||||
go test ./internal/framework/pipeline
|
||||
go test ./internal/modules/chunk/dnd/scenes
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- Config resolution can bind `dnd/scenes`.
|
||||
- User and internal docs describe the implemented module accurately.
|
||||
- Existing examples and CLI docs remain truthful.
|
||||
- `dnd/scenes` prompt/schema provenance is visible in durable
|
||||
`manifest.json` and diagnostics `run-manifest.json`.
|
||||
- Existing artifact lane metadata remains in the same JSON location as before.
|
||||
|
||||
## Stage 5: Full Verification
|
||||
## Stage 2: Canonical Source Units In Chunk Results
|
||||
|
||||
Goal: verify the complete feature across contracts, production wiring, docs, and
|
||||
the command entry point.
|
||||
Goal: preserve the chunker boundary contract while ensuring extractors always
|
||||
consume source document units, not rewritten units supplied by a chunker.
|
||||
|
||||
Design decision:
|
||||
|
||||
- Keep the chunker contract expressed in terms of `contracts.SourceChunk`.
|
||||
- Continue validating chunk IDs, source IDs, indexes, unit membership, unit
|
||||
uniqueness within each chunk, and source-ordering.
|
||||
- After validation, canonicalize chunk units by replacing each returned
|
||||
`SourceUnit` with a defensive copy of the matching source document unit.
|
||||
- Preserve `SourceChunk.Metadata` as module-owned chunk metadata.
|
||||
- Do not require full source coverage and do not reject overlap between chunks.
|
||||
- Do not preserve chunker-mutated per-unit text, kind, or metadata. A chunker
|
||||
that wants to add scene-level information must use `SourceChunk.Metadata`.
|
||||
|
||||
Code changes:
|
||||
|
||||
- Replace or extend `validateChunkResult` in
|
||||
`internal/framework/pipeline/chunk_validation.go` so it returns canonical
|
||||
chunks, for example:
|
||||
|
||||
```go
|
||||
func validateAndCanonicalizeChunkResult(doc *source.SourceDocument, chunks []contracts.SourceChunk) ([]contracts.SourceChunk, error)
|
||||
```
|
||||
|
||||
- Build a source-unit lookup from the validated source document.
|
||||
- For each chunk:
|
||||
- validate the existing generic invariants;
|
||||
- copy chunk ID, source ID, index, and chunk metadata;
|
||||
- replace the unit slice with cloned source units from the source document in
|
||||
the returned boundary/order.
|
||||
- Update the runner to use canonical chunks for all downstream extraction and
|
||||
merge behavior.
|
||||
- Ensure chunk metadata is cloned so later module or caller mutation cannot
|
||||
affect runner state.
|
||||
- Keep the implementation source-agnostic. Do not inspect transcript-specific
|
||||
metadata keys.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add a runner test where a fake chunker returns a valid unit ID with mutated
|
||||
text, kind, and unit metadata. Assert the extractor receives the original
|
||||
source document unit values.
|
||||
- Add a runner test proving chunk metadata survives canonicalization and is not
|
||||
aliased to the chunker-returned map.
|
||||
- Keep existing tests for invalid chunk IDs, duplicate IDs, wrong source ID,
|
||||
wrong index, empty units, repeated unit IDs, unknown unit IDs, out-of-order
|
||||
units, partial coverage, and overlap.
|
||||
- Add or update tests so generic chunking still behaves unchanged.
|
||||
|
||||
Documentation:
|
||||
|
||||
- Update internal chunk contract docs to state that source units in chunks are
|
||||
canonicalized from the source document by ID before extractors run.
|
||||
- Document that chunk metadata is the supported mechanism for passing
|
||||
chunker-owned context to extractors.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/framework/pipeline
|
||||
go test ./internal/modules/chunk/generic
|
||||
go test ./internal/modules/chunk/dnd/scenes
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- Extractors cannot observe chunker-rewritten source-unit text, kind, or unit
|
||||
metadata.
|
||||
- Chunker-owned scene metadata still reaches extractors through
|
||||
`SourceChunk.Metadata`.
|
||||
|
||||
## Stage 3: Scene Caveat Hygiene
|
||||
|
||||
Goal: ensure model caveats become useful warnings and never produce blank
|
||||
warnings that can confuse operators or affect diagnostics retention.
|
||||
|
||||
Design decision:
|
||||
|
||||
- Require caveat strings to be non-empty after trimming.
|
||||
- Treat whitespace-only caveats as malformed structured output rather than
|
||||
silently dropping them. This is consistent with the `dnd/scenes` policy of
|
||||
failing explicitly for malformed model output.
|
||||
- Store warning messages as trimmed caveat text.
|
||||
|
||||
Code changes:
|
||||
|
||||
- Update `internal/modules/chunk/dnd/scenes/assets/schemas/dnd_scenes.v1.json`
|
||||
so `boundary_caveats.items` has `minLength: 1`.
|
||||
- Update `warningsFromCaveats` or response validation in
|
||||
`internal/modules/chunk/dnd/scenes/chunker.go` to trim caveats and reject
|
||||
empty results with a module-prefixed malformed-output error.
|
||||
- Prefer validating caveats before constructing chunks so all malformed response
|
||||
checks happen together.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add schema tests proving `boundary_caveats` items require non-empty strings.
|
||||
- Add chunker tests proving:
|
||||
- caveat warning messages are trimmed;
|
||||
- whitespace-only caveats fail explicitly;
|
||||
- valid caveats still produce `scene_boundary_caveat` warnings.
|
||||
- Keep existing warning tests passing.
|
||||
|
||||
Documentation:
|
||||
|
||||
- Update `docs/internal/modules.md` and `docs/troubleshooting.md` if needed to
|
||||
mention that malformed caveats are treated as malformed structured output.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/modules/chunk/dnd/scenes
|
||||
go test ./internal/cli
|
||||
```
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- No blank warnings can be emitted from `dnd/scenes` boundary caveats.
|
||||
- Valid caveats remain visible as warnings.
|
||||
|
||||
## Stage 4: Full Verification
|
||||
|
||||
Goal: verify the follow-up work across contracts, production wiring,
|
||||
documentation, and the command entry point.
|
||||
|
||||
Run:
|
||||
|
||||
@@ -295,18 +229,17 @@ go vet ./...
|
||||
go build ./cmd/notarius
|
||||
```
|
||||
|
||||
Inspect diagnostics-sensitive output manually in tests or fixtures where
|
||||
relevant:
|
||||
Inspect or test representative output manifests:
|
||||
|
||||
- no raw prompts, source text, provider payloads, API keys, or secrets in
|
||||
manifest metadata;
|
||||
- errors name the module and operation;
|
||||
- warnings are preserved in `RunOutput.Warnings`;
|
||||
- run manifests record the `dnd/scenes` chunker when selected.
|
||||
- `manifest.json` includes `module_metadata.chunker` for a `dnd/scenes` run;
|
||||
- `module_metadata.chunker` contains prompt and response-schema provenance;
|
||||
- no raw prompt, raw schema, source text, provider payload, or secret appears in
|
||||
module metadata;
|
||||
- artifact lane metadata remains under `artifact_lanes[].metadata`;
|
||||
- `warnings.json` contains trimmed scene caveats and no blank caveat warnings.
|
||||
|
||||
Stage completion criteria:
|
||||
|
||||
- Full validation commands pass.
|
||||
- The feature is documented as implemented only where code supports it.
|
||||
- `docs/roadmap/chunk.md` retains target-state context and does not duplicate
|
||||
current-behavior reference material.
|
||||
- Current-behavior docs match implemented behavior.
|
||||
- Any remaining planned or deferred behavior stays under `docs/roadmap/`.
|
||||
|
||||
Reference in New Issue
Block a user