diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index cec1975..2347660 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -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/`.