Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions app/cli/internal/trace/spec/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,31 @@ import (
// delimiter opens and closes a frontmatter block.
const delimiter = "---"

// The limits of a title and a description, in characters. A model sometimes
// writes a paragraph where a label is asked for, and a cut keeps a usable one.
const (
MaxTitleLen = 120
MaxDescriptionLen = 300
)

// Meta holds what the agent states about a source next to its kind: its
// purpose, a short name, and what it holds. A text file carries it in its
// header, and a binary file in a companion file.
type Meta struct {
// Role is one of the aicodingsession.SpecRole* constants, or empty.
Role string `yaml:"role"`
// Title is a short name for the source, or empty.
Title string `yaml:"title"`
// Description says what the source holds, or is empty.
Description string `yaml:"description"`
}

// frontmatter is the header a spec document carries. It is decoded into a typed
// struct rather than a map so an unexpected key cannot reach the evidence.
type frontmatter struct {
Kind string `yaml:"kind"`
URI string `yaml:"uri"`
Meta `yaml:",inline"`
}

// Capture is one spec document as read from the session folder: what the agent
Expand All @@ -45,6 +65,9 @@ type Capture struct {
Kind string
// URI is where the text came from, empty for a spec stated in the session.
URI string
// Meta holds the role, the title and the description of the source. For
// a binary file they stay empty until its companion file is redacted.
Meta
// CapturedAt is the file's modification time, RFC3339.
CapturedAt string
// Raw is the file as the agent wrote it, header included. It is what
Expand All @@ -54,6 +77,9 @@ type Capture struct {
// image, or any file that is not text. The agent copied it into the
// folder, so it has no header and nothing in it is rewritten.
Verbatim bool
// MetaRaw is the companion file of a verbatim file as the agent wrote it,
// or nil when there is none. It is redacted before ParseMeta reads it.
MetaRaw []byte
}

// verbatimCapture describes a file that is stored as it is. Its kind comes
Expand Down Expand Up @@ -86,6 +112,8 @@ func verbatimCapture(name string, doc []byte, modTime time.Time, image bool) Cap
// - Frontmatter that is not valid YAML: the body after the block is content,
// again with kind "text" and no URI.
// - A kind outside the vocabulary: normalised to "text".
// - A role outside the vocabulary: no role.
// - A title or a description over its limit: cut to the limit.
//
// It returns nil when nothing is left once the body is trimmed. That is the
// common shape of a session with nothing to capture: the agent was told to
Expand All @@ -110,10 +138,42 @@ func Parse(doc []byte, capturedAt time.Time) *Capture {
return &Capture{
Kind: aicodingsession.ResolveSpecKind(meta.Kind),
URI: strings.TrimSpace(meta.URI),
Meta: meta.normalize(),
CapturedAt: capturedAt.UTC().Format(time.RFC3339),
}
}

// ParseMeta reads the companion file of a binary file. It never fails: a file
// that is not valid YAML gives no values, and costs the binary file nothing.
func ParseMeta(doc []byte) Meta {
var meta Meta
if err := yaml.Unmarshal(doc, &meta); err != nil {
return Meta{}
}

return meta.normalize()
}

// normalize drops a role outside the vocabulary, trims the title and the
// description, and cuts them to their limits.
func (m Meta) normalize() Meta {
return Meta{
Role: aicodingsession.ResolveSpecRole(m.Role),
Title: truncate(strings.TrimSpace(m.Title), MaxTitleLen),
Description: truncate(strings.TrimSpace(m.Description), MaxDescriptionLen),
}
}

// truncate keeps the first limit characters of s, so a cut never splits one.
func truncate(s string, limit int) string {
runes := []rune(s)
if len(runes) <= limit {
return s
}

return strings.TrimSpace(string(runes[:limit]))
}

// split separates a leading frontmatter block from the body. A document with no
// well-formed block is all body, so that a missing or unterminated header
// cannot swallow the text it was supposed to introduce.
Expand Down
76 changes: 76 additions & 0 deletions app/cli/internal/trace/spec/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ func TestParse(t *testing.T) {
wantKind string
wantURI string
wantContent string
wantRole string
wantTitle string
wantDesc string
}{
{
name: "a full frontmatter block",
Expand Down Expand Up @@ -118,6 +121,51 @@ func TestParse(t *testing.T) {
wantKind: aicodingsession.SpecKindImage,
wantContent: "what the mockup shows",
},
{
name: "a role, a title and a description",
doc: "---\nkind: ticket\nrole: task\ntitle: \"ENG-1234: Add an export button\"\ndescription: The ticket that the session implements.\n---\n" + specBody,
wantKind: aicodingsession.SpecKindTicket,
wantRole: aicodingsession.SpecRoleTask,
wantTitle: "ENG-1234: Add an export button",
wantDesc: "The ticket that the session implements.",
wantContent: specBody,
},
{
name: "a role with case and padding",
doc: "---\nrole: \" Reference \"\n---\n" + specBody,
wantKind: aicodingsession.SpecKindText,
wantRole: aicodingsession.SpecRoleReference,
wantContent: specBody,
},
{
// A bad role costs the role only: never the kind, and never a
// role guessed from the kind.
name: "a role outside the vocabulary",
doc: "---\nkind: document\nrole: design\n---\n" + specBody,
wantKind: aicodingsession.SpecKindDocument,
wantContent: specBody,
},
{
name: "a title and a description of only whitespace",
doc: "---\ntitle: \" \"\ndescription: \"\\t\"\n---\n" + specBody,
wantKind: aicodingsession.SpecKindText,
wantContent: specBody,
},
{
// The limit counts characters, so a cut never splits one.
name: "a title over the limit is cut",
doc: "---\ntitle: " + strings.Repeat("é", MaxTitleLen+5) + "\n---\n" + specBody,
wantKind: aicodingsession.SpecKindText,
wantTitle: strings.Repeat("é", MaxTitleLen),
wantContent: specBody,
},
{
name: "a description over the limit is cut",
doc: "---\ndescription: " + strings.Repeat("a", MaxDescriptionLen+1) + "\n---\n" + specBody,
wantKind: aicodingsession.SpecKindText,
wantDesc: strings.Repeat("a", MaxDescriptionLen),
wantContent: specBody,
},
{name: "an empty document", doc: "", wantNil: true},
{name: "frontmatter with no body", doc: "---\nkind: ticket\n---\n", wantNil: true},
{name: "a whitespace-only body", doc: "---\nkind: ticket\n---\n \n\n\t\n", wantNil: true},
Expand All @@ -136,6 +184,9 @@ func TestParse(t *testing.T) {
require.NotNil(t, got)
assert.Equal(t, tc.wantKind, got.Kind)
assert.Equal(t, tc.wantURI, got.URI)
assert.Equal(t, tc.wantRole, got.Role)
assert.Equal(t, tc.wantTitle, got.Title)
assert.Equal(t, tc.wantDesc, got.Description)
// What follows the header is the text: split decides where the
// header ends, which is also what decides the kind and the URI.
_, body := split(tc.doc)
Expand All @@ -144,3 +195,28 @@ func TestParse(t *testing.T) {
})
}
}

func TestParseMeta(t *testing.T) {
testCases := []struct {
name string
doc string
want Meta
}{
{
name: "every field",
doc: "role: reference\ntitle: Export button mockup\ndescription: Where the button goes.\n",
want: Meta{Role: aicodingsession.SpecRoleReference, Title: "Export button mockup", Description: "Where the button goes."},
},
{name: "a role outside the vocabulary", doc: "role: mockup\ntitle: x\n", want: Meta{Title: "x"}},
{name: "a title over the limit is cut", doc: "title: " + strings.Repeat("a", MaxTitleLen+1), want: Meta{Title: strings.Repeat("a", MaxTitleLen)}},
// A file we cannot read costs its values, and nothing else.
{name: "not valid YAML", doc: "role: plan\ntitle: [unclosed\n", want: Meta{}},
{name: "an empty file", doc: "", want: Meta{}},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.want, ParseMeta([]byte(tc.doc)))
})
}
}
24 changes: 23 additions & 1 deletion app/cli/internal/trace/spec/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ const (
// the oldest new ones, since what the session started from is the last
// thing worth dropping.
MaxEntries = 25

// MetaSuffix names the companion file of a binary file: the name of the
// binary file plus this suffix. It holds the role, the title and the
// description that a binary file has no header for.
MetaSuffix = ".meta.yaml"
)

// Dir returns the directory holding every session's captured specs.
Expand Down Expand Up @@ -156,12 +161,21 @@ func ReadAll(repoRoot, sessionID string, recorded []string) ([]Capture, []string

var warnings []string

// A companion file is never a spec of its own. It is read only for the
// binary file it is named for, and ignored when there is none.
companions := make(map[string]string)

candidates := make([]candidate, 0, len(dirEntries))
for _, e := range dirEntries {
if !isCandidate(e) {
continue
}

if base, ok := strings.CutSuffix(e.Name(), MetaSuffix); ok {
companions[base] = filepath.Join(dir, e.Name())
continue
}

info, err := e.Info()
if err != nil {
warnings = append(warnings, notRecorded(e.Name(), err))
Expand Down Expand Up @@ -214,7 +228,15 @@ func ReadAll(repoRoot, sessionID string, recorded []string) ([]Capture, []string
// An image, or a file that is not text, is something the agent copied
// in rather than wrote. Parsing it as text would only mangle it.
if image := isImage(c.name, doc); image || !utf8.Valid(doc) {
entries = append(entries, verbatimCapture(c.name, doc, c.modTime, image))
capture := verbatimCapture(c.name, doc, c.modTime, image)
if path, ok := companions[c.name]; ok {
// A companion file we cannot read costs its values only.
if meta, err := os.ReadFile(path); err == nil {
capture.MetaRaw = meta
}
}

entries = append(entries, capture)
continue
}

Expand Down
37 changes: 37 additions & 0 deletions app/cli/internal/trace/spec/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,43 @@ func TestReadAll(t *testing.T) {
assert.True(t, entries[1].Verbatim)
})

t.Run("a companion file goes with its binary file and is no spec of its own", func(t *testing.T) {
root := t.TempDir()
pngBytes := []byte("\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\xff\xfe")
meta := "role: reference\ntitle: Export button mockup\n"
writeSpec(t, root, sessionID, "mockup.png", string(pngBytes), time.Now())
writeSpec(t, root, sessionID, "mockup.png"+MetaSuffix, meta, time.Now())

entries, warnings, err := ReadAll(root, sessionID, nil)

require.NoError(t, err)
assert.Empty(t, warnings)
require.Len(t, entries, 1)
assert.Equal(t, "mockup.png", entries[0].FileName)
assert.Equal(t, []byte(meta), entries[0].MetaRaw)
// The values are read only after the companion file is redacted.
assert.Empty(t, entries[0].Title)
assert.Empty(t, entries[0].Role)
})

t.Run("a companion file with no binary file next to it is ignored", func(t *testing.T) {
root := t.TempDir()
writeSpec(t, root, sessionID, "ticket.md", "the ticket", time.Now())
// One with no file at all, and one next to a text file, which has a
// header of its own.
writeSpec(t, root, sessionID, "gone.png"+MetaSuffix, "role: reference\n", time.Now())
writeSpec(t, root, sessionID, "ticket.md"+MetaSuffix, "role: task\n", time.Now())

entries, warnings, err := ReadAll(root, sessionID, nil)

require.NoError(t, err)
assert.Empty(t, warnings)
require.Len(t, entries, 1)
assert.Equal(t, "ticket.md", entries[0].FileName)
assert.Nil(t, entries[0].MetaRaw)
assert.Empty(t, entries[0].Role)
})

t.Run("an image in a text format is kept as it is too", func(t *testing.T) {
// An SVG is valid UTF-8, so only its type tells it apart from a spec
// the agent wrote. Parsing and redacting it as text could break it.
Expand Down
12 changes: 11 additions & 1 deletion app/cli/pkg/action/trace_spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,23 @@ Resolve whatever the task points at — a ticket, a design document, an image fi
---
kind: ticket
uri: https://tracker.example.com/issue/ENG-1234
role: task
title: "ENG-1234: Add an export button"
description: The ticket that this session implements.
---

Set kind to one of ticket, document, image or text. An approved plan, or spec text that came from outside this conversation, is kind text. Set uri to the source URL or the path of a local file, or omit the uri line when the text has no other source.

Set role to the purpose of the source. If a source has more than one purpose, pick the role that tells why it is in this session. Use one of these values:
- task: the item that states the work to do.
- spec: a document that defines what to build.
- plan: a plan that the user approved.
- reference: supporting material, such as a screenshot or a background document.
Set title to a short name for the source, such as the ticket title. Set description to one or two sentences that tell what the source holds.

The transcript of this session already holds the conversation, so the user's request prompt is not a spec: do not capture it. Spec text that the user pastes, such as a ticket or a design document, is a spec.

An image is the one exception to writing text. If you can reach the image as a file — on disk, or at a URL you can download — copy the file itself into the folder with a copy or download command (cp, curl -o), keeping its extension and adding no frontmatter. Do not capture an image pasted into this conversation: you have no file for it.
An image is the one exception to writing text. If you can reach the image as a file — on disk, or at a URL you can download — copy the file itself into the folder with a copy or download command (cp, curl -o), keeping its extension and adding no frontmatter. Then write the role, title and description lines in a second file next to it. Give that file the name of the image plus .meta.yaml, for example mockup.png.meta.yaml. Do not capture an image pasted into this conversation: you have no file for it.

If the task changes, or a spec changes — also one that this session writes, such as a design note outside the repository — overwrite its file with the current content, or add another. If there is nothing to capture — a one-line request, a typo fix, a question, a passing remark — write nothing at all.

Expand Down
Loading
Loading