diff --git a/ingestion/internal/api/handler.go b/ingestion/internal/api/handler.go index 95621f8..6359f74 100644 --- a/ingestion/internal/api/handler.go +++ b/ingestion/internal/api/handler.go @@ -174,9 +174,6 @@ func writeHallNote(brainDir string, opts WriteNoteOptions) (string, error) { return "", fmt.Errorf("claim envelope: %w", err) } - if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { - return "", fmt.Errorf("create hall dir: %w", err) - } emitted := make(map[string]bool, 6) var fm strings.Builder @@ -228,6 +225,12 @@ func writeHallNote(brainDir string, opts WriteNoteOptions) (string, error) { if err != nil { return "", err } + // The directory is created only once the note is known to be writable: + // a refused write leaves nothing behind, not even an empty hall dir + // (brain#21; provenance refusal joins the envelope refusal here). + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return "", fmt.Errorf("create hall dir: %w", err) + } if err := os.WriteFile(dest, []byte(final), 0o644); err != nil { return "", fmt.Errorf("write: %w", err) } diff --git a/ingestion/internal/api/provenance_test.go b/ingestion/internal/api/provenance_test.go index e23cf68..ffb3a69 100644 --- a/ingestion/internal/api/provenance_test.go +++ b/ingestion/internal/api/provenance_test.go @@ -108,3 +108,15 @@ func TestRESTIngestStampsUnknown(t *testing.T) { assert.Contains(t, strings.SplitN(string(b), "---\n", 3)[1], "origin: unknown\n", p) } } + +// A refused write leaves nothing behind, not even an empty hall directory +// (brain#21's invariant, round 3 non-blocking finding). +func TestWriteNoteRefusedForProvenanceLeavesNoDirectory(t *testing.T) { + dir := t.TempDir() + _, err := api.WriteNote(dir, api.WriteNoteOptions{ + Content: "# x\n\nbody", Wing: "freshwing", Hall: "facts", Filename: "x", Origin: "jwt", WrittenBy: "bad\xff", + }) + require.Error(t, err) + _, statErr := os.Stat(filepath.Join(dir, "wiki", "freshwing")) + assert.True(t, os.IsNotExist(statErr), "no wing directory may be created by a refused write") +} diff --git a/ingestion/internal/mcp/provenance_escapes_test.go b/ingestion/internal/mcp/provenance_escapes_test.go index bf2e48a..0783f1c 100644 --- a/ingestion/internal/mcp/provenance_escapes_test.go +++ b/ingestion/internal/mcp/provenance_escapes_test.go @@ -2,6 +2,7 @@ package mcp_test import ( "bytes" + "context" "encoding/json" "log/slog" "os" @@ -20,6 +21,7 @@ import ( "github.com/mathiasbq/hyperguild/ingestion/internal/capturehttp" "github.com/mathiasbq/hyperguild/ingestion/internal/classification" "github.com/mathiasbq/hyperguild/ingestion/internal/mcp" + "github.com/mathiasbq/hyperguild/ingestion/internal/pipeline" ) // Escapes found by the PR #147 gatekeeper review. Each test drives the real @@ -260,3 +262,64 @@ func TestToolsCallAuditErrorOutcomeCarriesNoArgumentValues(t *testing.T) { } assert.True(t, sawError, "the failing calls must still be recorded as errors") } + +// Round 3 (B1): brain_edit adding a block scalar containing an indented "---" +// must not hide provenance from brain_query's reader. +func TestBrainEditBlockScalarFenceDoesNotHideProvenanceFromQuery(t *testing.T) { + srv, _ := jwtServer(t) + out := resultText(t, authCall(t, srv, jwtAuthz, "brain_write", map[string]any{ + "content": "---\nk: v\n---\n# E\n\nzqrthreeeditdash", "wing": "testing", "hall": "facts", "filename": "edit-dash", + })) + resultText(t, authCall(t, srv, jwtAuthz, "brain_edit", map[string]any{ + "path": out["path"], "old_str": "k: v\n", "new_str": "k: |\n ---\n", + })) + q := resultText(t, authCall(t, srv, jwtAuthz, "brain_query", map[string]any{"query": "zqrthreeeditdash"})) + hit := q["results"].([]any)[0].(map[string]any) + assert.Equal(t, "jwt", hit["origin"]) + assert.Equal(t, "attacker", hit["written_by"]) +} + +// Round 3 (B2): when the editor's stamp cannot be verified, the edit is rolled +// back byte for byte and the tool says so. A JWT subject with an invalid UTF-8 +// byte cannot round-trip through YAML, which forces that path. +func TestBrainEditRollsBackWhenTheStampCannotBeVerified(t *testing.T) { + dir := newBrainRepo(t) + cfg, err := classification.Load(dir) + require.NoError(t, err) + svc := capture.NewService(brainstore.New(dir), capFakeTracker{}, cfg, audit.NewSlogSink(nil)) + srv := mcp.NewServer(dir, nil, nil, nil) + srv.WithCapture(svc, capFakeValidator{subject: "bad\xff"}, capStaticTok, "local-cli", capturehttp.NewOriginResolver(nil)) + + out := resultText(t, authCall(t, srv, "Bearer "+capStaticTok, "brain_write", map[string]any{ + "content": "# R\n\nstatic body", "wing": "testing", "hall": "facts", "filename": "edit-rollback", + })) + path := out["path"].(string) + before, err := os.ReadFile(filepath.Join(dir, path)) + require.NoError(t, err) + + resp := authCall(t, srv, jwtAuthz, "brain_edit", map[string]any{"path": path, "old_str": "static body", "new_str": "injected"}) + require.NotNil(t, resp["error"], "the edit must be reported as failed") + assert.Contains(t, resp["error"].(map[string]any)["message"], "rolled back") + after, err := os.ReadFile(filepath.Join(dir, path)) + require.NoError(t, err) + assert.Equal(t, string(before), string(after), "the note must be byte-identical to before the edit") +} + +// MCP brain_ingest (the LLM path) attributes its pages to the caller. +func TestBrainIngestViaMCPStampsCaller(t *testing.T) { + dir := newBrainRepo(t) + cfg, err := classification.Load(dir) + require.NoError(t, err) + svc := capture.NewService(brainstore.New(dir), capFakeTracker{}, cfg, audit.NewSlogSink(nil)) + stub := pipeline.Config{Complete: func(context.Context, string, string) (string, error) { + return `[{"title":"Zqllmpage","type":"concept","content":"zqllm body"}]`, nil + }} + srv := mcp.NewServer(dir, &stub, nil, nil) + srv.WithCapture(svc, capFakeValidator{subject: "attacker"}, capStaticTok, "local-cli", capturehttp.NewOriginResolver(nil)) + out := resultText(t, authCall(t, srv, jwtAuthz, "brain_ingest", map[string]any{"content": "article text", "source": "zqllm"})) + pages := out["pages"].([]any) + require.NotEmpty(t, pages) + for _, p := range pages { + assertProvenance(t, srv, dir, p.(string), "jwt", "attacker") + } +} diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index 17f0d5b..bcfa12a 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -110,10 +110,17 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM // verified, the edit is rolled back rather than left on disk unattributed // (or carrying whatever provenance the edit wrote). var a brainEditArgs - _ = json.Unmarshal(args, &a) - var prior []byte - if t, rerr := notes.Resolve(s.brainDir, a.Path, notes.ScopeNotes); rerr == nil { - prior, _ = os.ReadFile(t.Abs) + if err := json.Unmarshal(args, &a); err != nil { + return nil, fmt.Errorf("parse args: %w", err) + } + target, err := notes.Resolve(s.brainDir, a.Path, notes.ScopeNotes) + if err != nil { + return nil, err + } + // No snapshot, no edit: without one a failed stamp could not be undone. + prior, err := os.ReadFile(target.Abs) + if err != nil { + return nil, fmt.Errorf("snapshot before edit: %w", err) } res, out, err := s.editWithScope(args, notes.ScopeNotes) if err != nil { @@ -124,8 +131,10 @@ func (s *Server) brainEdit(ctx context.Context, args json.RawMessage) (json.RawM // to the editor after every edit (PR #147 review, escape E1). origin, writtenBy := provenanceFrom(ctx) if serr := api.StampProvenanceFile(s.brainDir, res.Path, origin, writtenBy); serr != nil { - if t, rerr := notes.Resolve(s.brainDir, res.Path, notes.ScopeNotes); rerr == nil && prior != nil { - _ = os.WriteFile(t.Abs, prior, 0o644) + if werr := os.WriteFile(target.Abs, prior, 0o644); werr != nil { + // Say exactly what state the note is in: edited, unverified. + return nil, fmt.Errorf("stamp provenance failed (%v) AND the edit could NOT be rolled back: %w; "+ + "%s carries the edit under its previous attribution", serr, werr, res.Path) } return nil, fmt.Errorf("edit rolled back, stamp provenance: %w", serr) } diff --git a/ingestion/internal/provenance/provenance.go b/ingestion/internal/provenance/provenance.go index 62d4b8b..1343e0d 100644 --- a/ingestion/internal/provenance/provenance.go +++ b/ingestion/internal/provenance/provenance.go @@ -85,9 +85,15 @@ func Apply(content string, s Stamp) (string, error) { content = foldFrontmatterCR(content) block, rest, ok := sharedfm.Split([]byte(content)) var out string - if !ok { + switch { + case !ok && strings.HasPrefix(strings.ReplaceAll(content, "\r\n", "\n"), "---\n"): + // A fence was opened but cannot be closed (e.g. a "---x" line). + // Prepending a second block would demote the original frontmatter + // into the body, so refuse instead (PR #147 review round 3). + return "", fmt.Errorf("%w: unclosed frontmatter fence", ErrUnverifiable) + case !ok: out = "---\n" + stamp + "---\n" + content - } else { + default: out = "---\n" + plainGroups(string(block)) + stamp + "---\n" + strings.TrimPrefix(string(rest), "\n") } if err := verify(out, s); err != nil { @@ -203,11 +209,18 @@ func foldFrontmatterCR(content string) string { return strings.ReplaceAll(s[:4+end], "\r", "\n") + s[4+end:] } -// scalar renders a bare word as-is and anything else as a JSON string (valid -// YAML double-quoted), so a principal name can never break the line. +// scalar renders a bare word as-is only when YAML reads it back as that same +// string; anything else (true, 123, null, ~, punctuation, newlines) becomes +// a double-quoted string, so a principal name can never break the line or +// change type. func scalar(v string) string { if bareScalar.MatchString(v) { - return v + var back any + if yaml.Unmarshal([]byte(v), &back) == nil { + if s, ok := back.(string); ok && s == v { + return v + } + } } return fmt.Sprintf("%q", v) } diff --git a/ingestion/internal/provenance/provenance_test.go b/ingestion/internal/provenance/provenance_test.go new file mode 100644 index 0000000..c666c8a --- /dev/null +++ b/ingestion/internal/provenance/provenance_test.go @@ -0,0 +1,86 @@ +package provenance + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func fmOf(t *testing.T, doc string) map[string]any { + t.Helper() + parts := strings.SplitN(doc, "---\n", 3) + require.Len(t, parts, 3, doc) + var m map[string]any + require.NoError(t, yaml.Unmarshal([]byte(parts[1]), &m), parts[1]) + return m +} + +// The whole-document backstop: a stamp that would not read back as written +// is refused, never written. A principal with an invalid UTF-8 byte quotes +// to "\xff", which YAML decodes as U+00FF, so it cannot round-trip. +func TestApplyRefusesAStampThatDoesNotReadBack(t *testing.T) { + _, err := Apply("---\ntitle: t\n---\nbody\n", Stamp{Origin: "jwt", WrittenBy: "bad\xff"}) + assert.True(t, errors.Is(err, ErrUnverifiable), "got %v", err) +} + +// A principal that YAML would decode as a non-string (true, 123, null) is +// quoted, so the stamp reads back as the same string. +func TestApplyQuotesPrincipalsYAMLWouldNotReadAsStrings(t *testing.T) { + for _, p := range []string{"true", "123", "null", "1e3", "0x1F", "yes", "~"} { + out, err := Apply("---\ntitle: t\n---\nbody\n", Stamp{Origin: "jwt", WrittenBy: p}) + require.NoError(t, err, p) + assert.Equal(t, p, fmOf(t, out)["written_by"], p) + } +} + +// A group that does not decode on its own is dropped; its neighbours stay. +func TestApplyDropsGroupsThatDoNotDecodeAlone(t *testing.T) { + out, err := Apply("---\nkeep: 1\ntitle: a: b\nalso: 2\n---\nbody\n", Stamp{Origin: "static", WrittenBy: "local-cli"}) + require.NoError(t, err) + m := fmOf(t, out) + assert.Equal(t, 1, m["keep"]) + assert.Equal(t, 2, m["also"]) + assert.NotContains(t, m, "title") +} + +func TestApplyKeepsFirstOfDuplicateKeys(t *testing.T) { + out, err := Apply("---\ntitle: first\ntitle: second\nTitle: third\n---\nbody\n", Stamp{}) + require.NoError(t, err) + m := fmOf(t, out) + assert.Equal(t, "first", m["title"]) + assert.NotContains(t, m, "Title") +} + +func TestApplyFoldsLoneCRSoItCannotHideALine(t *testing.T) { + out, err := Apply("---\ntitle: x\rorigin: static\n---\nbody\n", Stamp{Origin: "jwt", WrittenBy: "a"}) + require.NoError(t, err) + assert.NotContains(t, out, "\r") + assert.Equal(t, "jwt", fmOf(t, out)["origin"]) + assert.Equal(t, "x", fmOf(t, out)["title"]) +} + +func TestApplyDropsTabIndentedContinuations(t *testing.T) { + out, err := Apply("---\ntitle: x\n\torigin: static\nk: v\n---\nbody\n", Stamp{Origin: "jwt", WrittenBy: "a"}) + require.NoError(t, err) + m := fmOf(t, out) + assert.Equal(t, "jwt", m["origin"]) + assert.Equal(t, "v", m["k"]) +} + +// Content that opens a fence the shared splitter cannot close (e.g. a +// "---x" line) is refused rather than given a second, prepended block that +// demotes the original frontmatter into the body. +func TestApplyRefusesAnUnclosableFrontmatterFence(t *testing.T) { + _, err := Apply("---\ntitle: t\n---x\norigin: static\n---\nbody\n", Stamp{Origin: "jwt", WrittenBy: "a"}) + assert.True(t, errors.Is(err, ErrUnverifiable), "got %v", err) +} + +func TestApplyAddsFrontmatterToContentWithoutAny(t *testing.T) { + out, err := Apply("# plain\n\norigin: static is prose\n", Stamp{Origin: "static", WrittenBy: "local-cli"}) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(out, "---\norigin: static\nwritten_by: local-cli\n---\n# plain")) +} diff --git a/ingestion/internal/search/provenance_test.go b/ingestion/internal/search/provenance_test.go index ecc15fa..4c61573 100644 --- a/ingestion/internal/search/provenance_test.go +++ b/ingestion/internal/search/provenance_test.go @@ -19,3 +19,11 @@ func TestExtractProvenanceIgnoresBodyLines(t *testing.T) { assert.Equal(t, "jwt", o) assert.Equal(t, "attacker", w) } + +// Round 3 (B1): an indented "---" inside a block scalar is content, not the +// closing fence — the same rule as frontmatter.Split. +func TestExtractProvenanceUsesTheSharedFenceRule(t *testing.T) { + o, w := extractProvenance("---\nk: |\n ---\norigin: jwt\nwritten_by: attacker\n---\nbody\n") + assert.Equal(t, "jwt", o) + assert.Equal(t, "attacker", w) +} diff --git a/ingestion/internal/search/search.go b/ingestion/internal/search/search.go index 0fdab19..c272a61 100644 --- a/ingestion/internal/search/search.go +++ b/ingestion/internal/search/search.go @@ -5,6 +5,7 @@ import ( "bufio" "context" "fmt" + sharedfm "github.com/mathiasbq/hyperguild/ingestion/internal/frontmatter" "log/slog" "os" "path/filepath" @@ -296,33 +297,32 @@ func hydrate(brainDir, relPath string) (Result, error) { } // extractProvenance reads origin/written_by from the leading frontmatter -// block only — a body line that happens to start with "origin:" is not -// provenance. +// block only, located with the shared fence rule (frontmatter.Split: a "---" +// at column 0) so an indented "---" inside a block scalar is content, not a +// closing fence (PR #147 review round 3). A body line is never provenance. func extractProvenance(content string) (origin, writtenBy string) { - scanner := bufio.NewScanner(strings.NewReader(content)) - inFrontmatter := false - for scanner.Scan() { - line := scanner.Text() - if strings.TrimSpace(line) == "---" { - if !inFrontmatter { - inFrontmatter = true - continue - } - break + block, _, ok := sharedfm.Split([]byte(content)) + if !ok { + return "", "" + } + for _, line := range strings.Split(string(block), "\n") { + if line == "" || line[0] == ' ' || line[0] == '\t' { + continue } - if !inFrontmatter { - return "", "" - } - key, val, ok := strings.Cut(line, ":") - if !ok || strings.HasPrefix(line, " ") || strings.HasPrefix(line, "\t") { + key, val, found := strings.Cut(line, ":") + if !found { continue } v := strings.Trim(strings.TrimSpace(val), `"'`) switch strings.TrimSpace(key) { case "origin": - origin = v + if origin == "" { + origin = v + } case "written_by": - writtenBy = v + if writtenBy == "" { + writtenBy = v + } } } return origin, writtenBy