feat(knowledge): carry the bundle's compile date in every envelope (BE-9893) - #828
Conversation
…E-9893) A consumer reading an enrichment envelope had no way to tell how old the advice was. `stale` looks like the answer and is not: it is true only on the stale-cache path, so a caller that passes COMFY_KNOWLEDGE_FILE always reads False no matter how old the file is. `as_of` looks like the answer and is not: it is the loaded file's mtime, which on the cloud agent is the time the pod warm-fetched the bundle, not the time the content was written. Read compiled_at from the manifest and ship it wherever bundle_version already goes: the attach block and marker, and the status, resolve, pick and capabilities payloads. It is None for a bundle compiled before the key existed, so old bundles keep loading. Additive, per the bundle schemas' additive-only rule. `as_of` and `stale` keep their meanings and gain comments saying what those meanings are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…very site Review of the previous commit found four gaps. Neither `comfy_cli/schemas/knowledge.json` nor `knowledge_block.json` declared the key, breaking the convention the three prior additive knowledge fields followed. Declared as ["string", "null"] and opaque: the producer validates the date, a consumer must not, which is the tolerant-reader rule the bundle schemas already state. Five of the six emit sites had no test. Deleting any one of them left the whole suite green. TestCompiledAtReachesEveryPayload now covers status, resolve, pick, the pick miss and the attach block, and deleting any single emit line fails at least one of them. Truncating an over-long value to MAX_VERSION_CHARS turned a hostile manifest into a date that parses to the wrong instant. Drop it instead: no date beats a wrong one. A version string tolerates the same treatment because nothing parses it. log_query now carries the date too. The miss log is the curation inbox, and a gap found against an old bundle should not rank beside one found against a current bundle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe knowledge bundle now loads an optional ChangesKnowledge bundle metadata
Merge Risk: ⚪ Minimal · up to This PR adds the bundle compile date to knowledge envelopes without changing existing field behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
skishore23
left a comment
There was a problem hiding this comment.
Approving. Additive and tolerant-reader throughout: every emit site (status, resolve, pick, pick-miss, attach block, miss marker, knowledge_query telemetry) has its own pinning test, the over-long-value guard drops rather than truncates (right call for a timestamp), None for pre-key bundles keeps 0.1.4 loading, and both schemas document the three date-ish fields against each other. The stale/as_of clarifying comments are worth having independent of the new field. Ran knowledge/attach/enrichment suites on the branch: 277 passed. Sequencing note acknowledged: inert (compiled_at: null) until comfy-knowledge#33 publishes a bundle carrying the key.
What changed
Every knowledge payload now carries
compiled_at, the bundle's content date, read frommanifest.json.Merge after Comfy-Org/comfy-knowledge#33, which is what writes the key. Until a bundle carrying it is published, this ships
compiled_at: nulleverywhere and looks inert.Why
A consumer reading an enrichment envelope had no way to tell how old the advice was. Two fields look like the answer and neither is one.
staleis true only on thestale-cachepath: the TTL expired and no fetch replaced it. A caller that setsCOMFY_KNOWLEDGE_FILEtakes theenvbranch, where it is hardcodedFalseno matter how old the file is. The cloud agent always sets that variable, sostaleis structurally alwaysFalsethere.as_ofis the loaded file's mtime. On the cloud agent that file is written by a GCS warm fetch at pod start, so it reports how long the pod has been up and says nothing about the content. Before the warm fetch shipped it was at least the image build time.Found triaging eval run
run_3e91922d97784821be2c96bf9a0debcf: every response carriedstale: falseon canon that was 21 days behind, and nothing in the envelope contradicted it.What this does not change
Additive, per the bundle schemas' additive-only rule.
as_ofandstalekeep their meanings and gain comments saying what those meanings are, so nothing that reads them today changes behaviour. A rename would also have put the code out of step with the design doc, which usesstalewith exactly this meaning in two places.compiled_atisNonefor a bundle compiled before the key existed, including the 0.1.4 bundle vendored into the cloud agent images, so old bundles keep loading.Where it ships
status,resolve,pick, the pick-capabilities miss, theattach()block and its miss marker, and theknowledge_querytelemetry event. The miss log is the curation inbox, so a gap found against an old bundle should not rank beside one found against a current bundle.Declared in
comfy_cli/schemas/knowledge.jsonandknowledge_block.jsonas["string", "null"]and documented as opaque. The producer validates the date; a consumer must not, which is the tolerant-reader rule the bundle schemas already state.An over-long value from a hostile manifest is dropped rather than truncated. Cutting a timestamp to length yields a date that parses to the wrong instant, and no date beats a wrong one. The
versionfield beside it tolerates truncation because nothing parses it.Testing
7289 passed, 39 skipped. ruff check and format clean on every file touched.TestCompiledAtReachesEveryPayloadcovers each emit site. Confirmed by deleting each"compiled_at": bundle.compiled_at,line and checking a test fails: without these, all six sites could be removed with the suite still green.End to end against a bundle compiled from comfy-knowledge#33:
as_ofthere is the capability row's curator date, a different field fromBundle.as_of. The point of the change is that these are now distinguishable.Follow-ups, both blocked on a pin bump
Cloud can stamp
compiled_atas a Langfuse span attribute besidebundle_versioninservices/agent/internal/loop/knowledge_span.go, and contractdata.compiled_atinservices/agent/internal/loop/cli_fields.json. Neither can land until this merges and the agent's comfy-cli pin moves.🤖 Generated with Claude Code