feat(effort-graph): add Citation + Blob with optional blob cites - #225
Conversation
Resolve the Blob collection issue by shipping Citation as the homogeneous `cites` target (Flatbread refs-safe). Citation body alone is valid (e.g. a URL); optional `blob` attaches longform payloads. Epistemic writes gain optional `cites`; digests omit Blob bodies by default. Decision: dec-ship-citation-collection-with-optional-blob--fyga3x876n7rcnmn Issue: iss-implement-blob-collection-and-crumb-graph-cites--g2c7m6j39we5xy3z Change-Id: Id73687f3c3224ca3eb77b4fc870815811ebd754d
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 3 minutes 48 seconds in the queue, including 3 minutes 34 seconds running CI. Required conditions to merge
|
|
@Mergifyio queue |
☑️ Command
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review verdict
REQUEST_CHANGES — consensus HIGHs on silent cites drop for Citation/Blob writes, incomplete same-effort cite/blob validation vs read-path filtering, and missing CLI/live write→read lock-in for cit-/blb-.
Adversarial DAG (5 perspectives + judge) on 5754e36 vs main. Not dependency-only.
Blocking themes
- Silent field drop — Zod strips unknown keys on
WriteCitation/WriteBlob; planner alsodeletescites/edge fields. Callers get success with no cites persisted. - Write vs read contract —
assertCites/Citation.blobcheck kind only; other edges throwDifferent effort, whilerelations()filters cross-effort away (returned: 0). - Coverage — CLI/live tests mostly mkdir
citations/blobs; no write→read round-trip for cites/Citation.blob. - Docs drift —
SKILL.mdstill opens with six primitives / 13 mutations and a six-folder layout while later text says 15 mutations (opening lines outside this diff hunk — fix in follow-up edit).
Coverage plan (priority)
schemas.test.ts— negative: WriteCitation/WriteBlob withcites→ reject (.strict()), not stripplanner.test.ts— negative: unknown/blb-/cross-effort cites; foreign/missing/cross-effortCitation.blobeffort.test.ts— positive: WriteBlob → WriteCitation(blob) → WriteFinding(cites) → get/records/relationseffort.test.ts— negative: invalid cites rejected at CLIliveServerEffortGraph.test.ts— positive: GraphQLcites { id }+Citation.blob { id }after mutations
Reviewer scoreboard
- correctness-and-contracts: 5 findings, 3 gaps, signal:HIGH
- test-coverage-robustness: 6 findings, 6 gaps, signal:HIGH
- cli-and-runtime: 5 findings, 3 gaps, signal:HIGH
- effort-graph-schema-and-cites: 5 findings, 1 gap, signal:HIGH
- docs-and-positioning: 5 findings, 0 gaps, signal:HIGH
Sent by Cursor Automation: Flatbread PR Review
| export const WriteCitationSchema = z.object({ | ||
| type: z.literal('WriteCitation'), | ||
| ...common, | ||
| effort: id, | ||
| /** Optional longform/payload target; body alone (e.g. a URL) is valid. */ | ||
| blob: id.optional(), | ||
| role: z.string().min(1).optional(), | ||
| }); | ||
| export const WriteBlobSchema = z.object({ | ||
| type: z.literal('WriteBlob'), | ||
| ...common, | ||
| effort: id, | ||
| kind: z.string().min(1).optional(), | ||
| }); |
There was a problem hiding this comment.
HIGH (consensus) — WriteCitationSchema / WriteBlobSchema are plain z.objects, so unknown keys like cites / derives_from are stripped by EffortGraphMutationSchema.parse (CLI path) with no error.
Minimal fix: .strict() (or explicit reject) on these schemas; add negative parse tests for unknown edge fields and malformed blob / role / blob: null.
| function assertCites( | ||
| get: ( | ||
| id: string | ||
| ) => NonNullable<ReturnType<EffortGraphSnapshot['getRecord']>>, | ||
| cites: string[] | undefined | ||
| ): void { | ||
| for (const citeId of cites ?? []) { | ||
| const target = get(citeId); | ||
| if (target.kind !== 'citation') | ||
| throw new EffortGraphValidationError( | ||
| `cites must target a Citation, got ${target.kind} (${citeId})` | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
HIGH (consensus) — assertCites only requires target.kind === 'citation'. ResolveIssue / SetRiskState throw Different effort for cross-effort refs, but cites do not — then relations() filters them out (effort !== effortId → empty).
Minimal fix: Same-effort check here (mirror those mutations); add negatives for unknown cit id, cites: [blb-…], and cross-effort cit with exact error strings.
| if (kind === 'citation' && raw.blob !== undefined) { | ||
| const blob = get(raw.blob); | ||
| if (blob.kind !== 'blob') | ||
| throw new EffortGraphValidationError( | ||
| `Citation.blob must target a Blob, got ${blob.kind}` | ||
| ); | ||
| } | ||
| if (EPISTEMIC_CREATE.has(kind) || kind === 'effort') | ||
| assertCites(get, raw.cites); |
There was a problem hiding this comment.
HIGH (consensus) — Citation.blob validates kind only. Cross-effort blob attachment is allowed at write time; wrong-kind / missing / effort-mismatch messages are under-tested.
Minimal fix: Reject when blob.frontmatter.effort !== citation.effort; planner tests for foreign kind, unknown blob, and effort mismatch.
| if (kind === 'blob' || kind === 'citation') { | ||
| delete fm.cites; | ||
| delete fm.derives_from; | ||
| delete fm.supersedes; | ||
| delete fm.invalidates; | ||
| } |
There was a problem hiding this comment.
HIGH — On citation/blob creates, unsupported edge fields are silently deleted. Combined with Zod strip (or writer.mutate(input: any) bypassing Zod), callers can pass cites and get a successful write with nothing persisted.
Minimal fix: Throw EffortGraphValidationError if these fields are present instead of deleting them.
| export const CreateEffortSchema = z.object({ | ||
| type: z.literal('CreateEffort'), | ||
| ...common, | ||
| ...cites, |
There was a problem hiding this comment.
HIGH — CreateEffort accepts cites[], but Citations need an existing effort. Same-effort effort-level cites cannot be set at create time, and there is no post-create mutation to add them (cross-effort-only in practice).
Minimal fix: Remove cites from CreateEffort, or add an update path; until then docs must not imply same-effort effort-level cites work at create.
| 'citations', | ||
| 'blobs', |
There was a problem hiding this comment.
HIGH (consensus) — Tests mkdir citations/blobs but never assert WriteBlob → WriteCitation(blob) → WriteFinding(cites) → get / records / relations --relations cites.
Minimal fix: One serial happy-path round-trip plus one invalid-cites CLI reject.
| 'citations', | ||
| 'blobs', |
There was a problem hiding this comment.
HIGH — Live GraphQL path never asserts cites { id } / Citation.blob { id } after mutation (dirs only in this touch).
Minimal fix: Extend read-your-writes with WriteCitation + cited finding and a GraphQL query on cites and blob.
| 'citation', | ||
| 'blob', |
There was a problem hiding this comment.
HIGH (consensus) — Default effortRecords kinds now include citation/blob, but without CLI/live write→read lock-in that cites survive GraphQL→toRecord→digest/relations. Invalid --kinds failing open to empty results is a related gap.
Minimal fix: Round-trip coverage (see review body); validate --kinds against PrimitiveKind and raise EFFORT_GRAPH_INVALID_ARGUMENT on typos.
| @@ -49,7 +49,7 @@ The write journal is `<root>/.journal/`; read digests cache under | |||
|
|
|||
There was a problem hiding this comment.
HIGH — Prerequisites still list only six record folders (efforts…risks). Opening copy above the hunk still says “Six primitives” / “13 typed mutations” while this file later documents 15 mutations and Citation/Blob.
Minimal fix: Extend this path list to include citations/blobs, and update the opening paragraph + YAML description to eight collections / 15 mutations (sync packages/effort-graph/skills/… copy).
2 participants
Summary
Implements the Blob collection issue from #224 by adding Citation as the homogeneous
citestarget (keeps Flatbread corerefsstrength) and Blob as an optional longform payload behind a Citation.Model
WriteCitation/WriteBlobmutations (15 total)cites: Citation[]on all epistemic createseffort getzooms inDecision:
dec-ship-citation-collection-with-optional-blob--fyga3x876n7rcnmnIssue:
iss-implement-blob-collection-and-crumb-graph-cites--g2c7m6j39we5xy3z(resolved)Checklist
Backwards compatible?
cites→ Citation). Consumers must use the updatedeffortGraphContent()preset (includescitations/+blobs/).