feat: hledger parsing#25
Conversation
📝 WalkthroughWalkthroughThis PR introduces a major refactoring of the dispatch and access-control system from facet-keyed dispatch to token-based document/facet access, adds a new plug_dayledger plugin for hledger journal parsing and transaction claim matching, updates plug_plabels and plug_test to adopt the new ACL model, and extends the type system with facet-field-matching predicates and explicit routine-level document ACLs. ChangesCore Dispatch & Access Control Refactoring
Existing Plugin Adaptations
New plug_dayledger Plugin
Sequence Diagram(s)sequenceDiagram
participant Triage
participant Dispatch
participant WashPlugin as Wash Plugin Host
participant WflowGuest
participant FacetStorage
Triage->>Dispatch: emit ProcessorEvent with changed_facet_keys
Dispatch->>Dispatch: create DocRoutine with branch_path, staging_branch
Dispatch->>WashPlugin: get_args() for facet routine
WashPlugin->>WashPlugin: build_doc_facet_tokens(primary_doc)
WashPlugin->>WashPlugin: resolve config_docs, build tokens per owner
WashPlugin->>WflowGuest: pass FacetRoutineArgs (primary_doc, config_docs, invocation)
WflowGuest->>WflowGuest: locate facets by key with rights checks
WflowGuest->>FacetStorage: read facet data via token
WflowGuest->>FacetStorage: clone token, perform operations
WflowGuest->>FacetStorage: update facet via token.update()
Dispatch->>Dispatch: merge staging_branch, resolve heads
Dispatch-->>Triage: dispatch complete
sequenceDiagram
participant WflowCtx
participant JournalNote as Note Data
participant Parser as Hledger Parser
participant Matcher as Claim Matcher
participant ClaimStorage as Claim Facet
WflowCtx->>JournalNote: read note facet (journal text)
WflowCtx->>Parser: parse_journal(text)
Parser-->>WflowCtx: Vec<Transaction>
WflowCtx->>WflowCtx: construct src_ref from note URL/heads
WflowCtx->>ClaimStorage: locate claim tag with create rights
WflowCtx->>ClaimStorage: read existing claims as HashMap
WflowCtx->>Matcher: match_claims(transactions, existing, src_ref)
Matcher->>Matcher: hash/code matching, heuristic scoring
Matcher-->>WflowCtx: Vec<(id, Claim)> updated claims
WflowCtx->>ClaimStorage: serialize claims, create/update facets
WflowCtx-->>WflowCtx: return Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/plug_plabels/lib.rs (1)
213-262:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDoc-ACL predicate appears to contradict the processor predicate for
label-noteandlabel-image.The
label-noteprocessor (lines 487-503) andlabel-imageprocessor (lines 511-525) both trigger only when the doc does not havePseudoLabel. However, the corresponding routines'doc_aclshere havedoc_predicate: DocPredicateClause::HasTag(PseudoLabel)and grantPseudoLabelaccess only withcreate: false, write: true.If
RoutineDocAcl.doc_predicategates whether the per-doc facet ACL applies (which is consistent with howbuild_doc_facet_tokensinsrc/daybook_core/rt/wash_plugin.rslines 686-711 only mints tokens for facets matching the ACL on the doc):
- When the processor fires on a doc lacking
PseudoLabel, the doc_acl predicateHasTag(PseudoLabel)won't match.- No
PseudoLabel/PseudoLabelErrorFacettoken is granted to the routine.label_image.rsline 23-27 andlabel_note.rswould fail with "facet token with update rights not found", terminating every dispatch.Even if the predicate were satisfied,
create: falsewould still prevent the routines from addingPseudoLabelto a doc that doesn't yet have one — which is the entire purpose oflabel-note/label-image. Thelearn-*-label-candidatesroutines do correctly useHasTag(PseudoLabel)since they only operate on already-labeled docs.Likely intent for
label-note/label-image:
doc_predicate: a positive predicate matching the source content (e.g.,HasTag(Note)for label-note,HasTag(Blob)for label-image), or no predicate gating at all.create: trueforPseudoLabel(andPseudoLabelErrorFacet).#!/bin/bash # Verify how doc_predicate / doc_acls are evaluated and whether they gate facet-token minting. rg -nP --type=rust -C5 '\bdoc_predicate\b' rg -nP --type=rust -C3 '\bdoc_acls\b' ast-grep --pattern 'RoutineDocAcl { $$$ }' # Check whether create=false actually blocks facet creation in the runtime. rg -nP --type=rust -C5 '\bRoutineFacetAccess\b' rg -nP --type=rust -C3 '\bcreate:\s*(true|false)'Also applies to: 272-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_plabels/lib.rs` around lines 213 - 262, The RoutineDocAcl currently uses DocPredicateClause::HasTag(PseudoLabel) and sets create: false for PlabelFacetTag::PseudoLabel and PlabelFacetTag::PseudoLabelErrorFacet which prevents label-note and label-image from receiving/create tokens; update the doc_acls for the label-note and label-image routines so the doc_predicate matches the source docs (e.g., DocPredicateClause::HasTag(Note) for label-note and DocPredicateClause::HasTag(Blob) for label-image or remove the predicate entirely) and set create: true on the RoutineFacetAccess entries for PlabelFacetTag::PseudoLabel and PlabelFacetTag::PseudoLabelErrorFacet (ensure read/write/create flags reflect that the routines must be able to create and write pseudo-label facets), and mirror the same change in the other ACL block(s) covering the same routines.src/plug_plabels/wflows/label_engine.rs (1)
175-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't read the config facet through a potentially write-only token.
This branch prefers
rw_config_tokenand immediately callsheads()/get()on it, even thoughapply_labelingonly needs read access here. With the new split ACL model, an update-only config grant will fail this path even when a readablero_config_tokenis available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_plabels/wflows/label_engine.rs` around lines 175 - 214, The code currently prefers req.rw_config_token and calls heads()/get() on it even when only read access is needed; change the logic to prefer the read-only token so write-only grants won't be used for reads. Reorder the if branches to check req.ro_config_token first (and perform the heads()/get()/serde parsing into PseudoLabelCandidatesFacet there), then fall back to req.rw_config_token only if ro is None, and finally to default_label_set(); keep the existing error handling and use of heads_json, label_set (PseudoLabelCandidatesFacet), and default_label_set() unchanged.src/daybook_core/rt.rs (1)
973-979:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack merge success separately from workflow success.
merged_successfullyis now justJobRunResult::Success. Later,command_invoke_reply_from_resultinterpretsfalseas a post-run merge failure, so cancelled/terminal/worker errors on child commands get reported back as"kind":"merge-failed"instead of their real cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/rt.rs` around lines 973 - 979, The code sets merged_successfully by treating any JobRunResult::Success as a successful merge, which misclassifies cancelled/worker/terminal failures as "merge-failed"; change the merged_successfully binding to reflect the actual merge outcome (e.g., inspect a merge-specific field or enum variant on event.result such as a dedicated MergeSuccess/MergeFailed variant or an explicit merge_success boolean) instead of matching any JobRunResult::Success, and update the call site that passes event.result into command_invoke_reply_from_result so it receives the true merge status; if the JobRunResult enum lacks a merge-specific variant/field, add one (or add an explicit merge_success flag on the result) and use that when computing merged_successfully and when invoking command_invoke_reply_from_result.
🟡 Minor comments (6)
src/plug_dayledger/wit/deps/wasi-keyvalue-0.2.0-draft/package.wit-227-227 (1)
227-227:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocs/API drift: CAS is advertised but not exposed.
Line 227 states atomic
incrementand CAS are available, butinterface atomicscurrently only definesincrement. Please align docs or add CAS before this interface is relied on by consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/wit/deps/wasi-keyvalue-0.2.0-draft/package.wit` at line 227, Docs claim both atomic `increment` and CAS exist, but the `interface atomics` only exposes `increment`; add a CAS operation or update the docs. Fix by adding a compare-and-swap function (e.g., `cas` or `compare_and_swap`) to the `interface atomics` alongside `increment` with a signature that accepts a key/slot, expected value and new value and returns a boolean or previous value (match the project's wit convention), and update the documentation comment that mentions CAS if you choose to remove it instead; reference `interface atomics` and the existing `increment` symbol when making the change.CONTRIBUTING.md-73-73 (1)
73-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in plug description.
Line 73 has a spelling error:providign→providing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 73, Update the description for `./src/plug_plabels/` by fixing the typo: change "providign" to "providing" so the line reads "`./src/plug_plabels/`: plug providing pseudo-labelling and classification utils."src/plug_dayledger/hledger/claim_matcher.rs-597-607 (1)
597-607:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAppend prior
src_reftosrc_refswhen re-matching an existing claim.When an existing claim is re-matched from a new source,
build_claimpreservesexisting.src_refsbut dropsexisting.src_ref. This breaks the source ancestry chain. Based on the test assertion "first parse should not carry prior src refs" (which confirmssrc_refsaccumulates on re-parses), the intended semantics are thatsrc_refsrecords the chain of all prior sources.On re-match, append
existing.src_refto the front ofsrc_refsbefore returning the new claim:src_refs: { let mut refs = existing .map(|existing_claim| existing_claim.src_refs.clone()) .unwrap_or_default(); if let Some(existing_claim) = existing { refs.insert(0, existing_claim.src_ref.clone()); } refs }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/claim_matcher.rs` around lines 597 - 607, The Claim construction in build_claim (or the function creating the Claim) preserves existing.src_refs but omits existing.src_ref, breaking source ancestry; change the src_refs initialization so when existing.is_some() you clone existing.src_refs (or default), then insert existing.src_ref at the front (index 0) before assigning to the Claim.src_refs field, ensuring src_ref is appended into the accumulated src_refs on re-match.src/plug_dayledger/hledger/parse/common.rs-109-116 (1)
109-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSupport multi-word account names or document as v1 constraint.
The
accountnamepparser stops at the first space due tonone_of([' ', ...]), preventing accounts likeliabilities:credit cardfrom parsing correctly. The fixture filesquickstart.journalandsample2.journalcontain such accounts, but they are not currently tested. Either extend the parser to support single spaces in account names (stopping at double-space or tab before amounts per hledger spec), or document this single-word limitation in a module-level comment to prevent unintended data loss when processing real-world journals.src/plug_test/e2e/capability_regression_wflow.rs-369-370 (1)
369-370:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTautological assertion provides no coverage.
assert!(tag_keys.is_empty() || !tag_keys.is_empty())is always true regardless oftag_keyscontent. Same pattern at line 439. Either remove these lines or replace with the actual expected condition (e.g.,assert!(tag_keys.is_empty(), "minimal should have no primary tags")or document the indeterminate expectation).🐛 Suggested fix
- let tag_keys: Vec<String> = serde_json::from_value(report["primary_tag_keys"].clone())?; - assert!(tag_keys.is_empty() || !tag_keys.is_empty()); + // primary_tag_keys is intentionally unconstrained for the minimal report. + let _tag_keys: Vec<String> = serde_json::from_value(report["primary_tag_keys"].clone())?;(apply equivalent change at line 439)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_test/e2e/capability_regression_wflow.rs` around lines 369 - 370, The assertion assert!(tag_keys.is_empty() || !tag_keys.is_empty()) is tautological and should be replaced with a meaningful check: update the assertion on the variable tag_keys to reflect the expected behavior (for example use assert!(tag_keys.is_empty(), "expected no primary tags for minimal case") if the test expects no tags, or assert!(!tag_keys.is_empty(), "expected primary tags to be present") if tags must exist), and apply the same replacement for the other occurrence at the later check (the second tag_keys assertion around line 439) so both assertions validate the actual expected condition rather than always-true logic.src/daybook_core/wit/main.wit-110-117 (1)
110-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
facet-tag-token::createreturnsupdate-doc-errorwhile sibling lookups returnaccess-error— authorization denials conflated withother.
list_facetsandgetonfacet-tag-tokensurfaceaccess-errorwith an explicitdeniedvariant, butcreateandget_create_tokenreturnupdate-doc-error, which lacks adeniedcase. When authorization fails on create (line 909-912), the implementation returnsUpdateDocError::Other("create denied"), masking the denial as a generic error alongsideinvalid-patchandinvalid-key. Consider adding adeniedvariant toupdate-doc-erroror returning a union type likeresult<..., variant { access(access-error), update(update-doc-error) }>for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/wit/main.wit` around lines 110 - 117, The create/get-create-token methods on facet-tag-token currently return update-doc-error which lacks an access-denied case (the implementation even maps authorization failures to UpdateDocError::Other), causing denied authorizations to be masked; update the WIT and implementation to make authorization semantics consistent by either adding a denied variant to update-doc-error (and return UpdateDocError::Denied from facet-tag-token::create and facet-tag-token::get-create-token) or change those method signatures to return a union type like result<..., variant { access(access-error), update(update-doc-error) }>, and update the corresponding implementation sites (where UpdateDocError::Other("create denied") is produced) to return the new denied/access variant instead.
🧹 Nitpick comments (26)
src/plug_dayledger/wit/deps/wasmcloud-postgres-0.1.1-draft/package.wit (1)
324-334: ⚡ Quick winAlign
preparedocs with actual signatureThe comment references a
connection-tokenandconnection.wit, butprepareonly acceptsstatement: string. Please update the doc block to match this API contract to avoid integration mistakes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/wit/deps/wasmcloud-postgres-0.1.1-draft/package.wit` around lines 324 - 334, The docblock for the prepare function is out of sync with its signature: prepare(statement: string) does not take a connection-token and there is no direct reference to connection.wit in this API, so update the comment for prepare (and any mention of prepared-statement-token / statement-prepare-error) to remove references to a connection-token and connection.wit and instead document that prepare only accepts a SQL statement string, requires parameterized queries using $<integer>, and returns a prepared-statement-token or statement-prepare-error; keep the existing example and parameterization requirement but align wording to the single-argument signature of prepare.src/daybook_types/Cargo.toml (1)
76-76: ⚡ Quick winConsider making
jsonpath-rustan optional dependency tied to themanifestfeature.
jsonpath-rustis added as an unconditional dependency on a foundational types crate consumed across the entire workspace. According to the AI summary, it's used inreference.rsfor ACL/manifest evaluation, which maps naturally to themanifestfeature. Leaving it mandatory forces every consumer ofdaybook_types—even those that only need basic doc types—to compile the JSONPath engine.♻️ Proposed change
-jsonpath-rust = "1.0.4" +jsonpath-rust = { version = "1.0.4", optional = true }Then add
dep:jsonpath-rustto themanifestfeature:-manifest = ["dep:garde", "dep:semver", "schemars"] +manifest = ["dep:garde", "dep:semver", "schemars", "dep:jsonpath-rust"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_types/Cargo.toml` at line 76, Make jsonpath-rust optional and tied to the manifest feature: in Cargo.toml mark the dependency jsonpath-rust = { version = "1.0.4", optional = true } and add the features entry manifest = ["dep:jsonpath-rust"] (or add "dep:jsonpath-rust" to the existing manifest feature). Also guard runtime usage in reference.rs (the ACL/manifest evaluation code) behind cfg(feature = "manifest") so that code only references jsonpath-rust when the manifest feature is enabled.src/daybook_types/manifest.rs (2)
865-883: 💤 Low valueSilently swallowing
JsonPath::queryerrors loses authoring feedback.The existing
facet_has_reference_to_tag(L832-838) logs atdebuglevel whenselect_json_path_valuesfails. The newevaluate_facet_field_matchuseslet Ok(found) = raw.query(json_path) else { continue; };with no log, so a typo in a manifest'sjson_pathbecomes a silent "predicate never matches" with no diagnostic. Mirror the existing pattern for consistency and easier debugging of authored manifests.♻️ Add a debug log on parse/query error
for (_, raw) in facets { - let Ok(found) = raw.query(json_path) else { - continue; - }; + let found = match raw.query(json_path) { + Ok(values) => values, + Err(err) => { + debug!(error = %err, %json_path, "invalid facet field match json_path"); + continue; + } + }; for found_val in found { if compare_json_values(found_val, operator, expected) { return true; } } }As per coding guidelines: "Don't swallow errors with empty catch statements or catch-and-log unless explicitly told to do so" — here a debug log of the parse failure is the established pattern in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_types/manifest.rs` around lines 865 - 883, The function evaluate_facet_field_match currently swallows JsonPath query errors silently; update it to mirror the existing pattern used by facet_has_reference_to_tag/select_json_path_values by logging parse/query failures at debug level instead of continuing silently. When raw.query(json_path) returns Err, call debug! including the json_path, the error (from the Err), and a short context (e.g., the facet key or that the facet query failed) so manifest authors get diagnostics; keep the rest of the loop behavior the same.
885-911: ⚡ Quick winNumeric comparisons via
as_f64lose precision for large integer values.For hledger journals — the motivating use case for this PR — monetary amounts are typically encoded as integer cents/minor units, and queries like
amount > 1_000_000_00over those fields will silently misbehave once values exceed2^53.serde_json::Value::as_f64()succeeds onNumber::from(i64::MAX)but loses precision.Consider preferring exact integer comparisons when both sides are integers, falling back to f64 only for mixed/float operands:
♻️ Suggested ordering
CompareOp::Gt | CompareOp::Gte | CompareOp::Lt | CompareOp::Lte => { + if let (Some(lhs), Some(rhs)) = (lhs.as_i64(), rhs.as_i64()) { + return match op { + CompareOp::Gt => lhs > rhs, + CompareOp::Gte => lhs >= rhs, + CompareOp::Lt => lhs < rhs, + CompareOp::Lte => lhs <= rhs, + _ => unreachable!(), + }; + } + if let (Some(lhs), Some(rhs)) = (lhs.as_u64(), rhs.as_u64()) { + return match op { + CompareOp::Gt => lhs > rhs, + CompareOp::Gte => lhs >= rhs, + CompareOp::Lt => lhs < rhs, + CompareOp::Lte => lhs <= rhs, + _ => unreachable!(), + }; + } if let (Some(lhs), Some(rhs)) = (lhs.as_f64(), rhs.as_f64()) {A related minor:
Eq/NeuseValue's derivedPartialEq, sojson!(1) == json!(1.0)isfalse. If users write predicates withvalue: 1against a facet that stores1.0, the match fails silently. Worth either documenting this constraint onFacetFieldMatch::valueor normalizing both sides to a common numeric repr before comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_types/manifest.rs` around lines 885 - 911, compare_json_values currently uses as_f64 for ordered comparisons which loses precision for large integers; update compare_json_values to first detect integer numbers via serde_json::Number methods (use as_i64/as_u64) and perform exact integer comparisons when both lhs and rhs are integers (handle signed/unsigned mixing by promoting safely to i128/unsigned logic or compare via i128/u128), fall back to as_f64 only when one or both operands are non-integer floats or cannot be represented as integers, and also consider normalizing numeric equality in CompareOp::Eq/Ne to treat numeric equivalence (e.g., 1 and 1.0) consistently by comparing numbers via the same numeric normalization logic rather than relying solely on Value's PartialEq; focus your edits inside the compare_json_values function and update the match arms for CompareOp::{Eq,Ne,Gt,Gte,Lt,Lte} accordingly.src/daybook_wflows/wflows/ocr_image.rs (1)
35-37: 💤 Low valueDownscope the cloned blob token to READ-only.
clone(None)preserves the parent token's full rights. Sincemltools_ocr::ocr_imageonly requires read access, explicitly downscope toFacetRights::READto follow least-privilege principles and make the intent clear at the call site.♻️ Proposed downscope
- let blob_ocr_token = blob_facet_token.clone(None).map_err(|err| { - JobErrorX::Terminal(ferr!("access error cloning blob facet token: {err:?}")) - })?; + let blob_ocr_token = blob_facet_token + .clone(Some(FacetRights::READ)) + .map_err(|err| { + JobErrorX::Terminal(ferr!("access error cloning blob facet token: {err:?}")) + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_wflows/wflows/ocr_image.rs` around lines 35 - 37, The cloned blob token currently uses blob_facet_token.clone(None) which preserves full rights; change the clone call to explicitly downscope to read-only by cloning with FacetRights::READ so blob_ocr_token supplies only read access to mltools_ocr::ocr_image and update the map_err message if needed; locate the clone invocation that produces blob_ocr_token (and references blob_facet_token) and replace the None scope with FacetRights::READ to enforce least-privilege for the ocr_image call.src/plug_dayledger/hledger/tests/mod.rs (3)
162-166: ⚡ Quick win
test_sample_journal_not_emptyis redundant.It only verifies the bundled fixture file is not empty, which is enforced at compile time by
include_str!(any missing path errors out at build) and is then re-tested bytest_parse_sample_journal_fixture. Drop this test to reduce noise.♻️ Suggested removal
-#[test] -fn test_sample_journal_not_empty() { - let input = include_str!("fixtures/sample.journal"); - assert!(!input.is_empty()); -} - #[test] fn test_parse_sample_journal_fixture() {As per coding guidelines: "remove useless or shallow tests and consolidate repeated setup code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/tests/mod.rs` around lines 162 - 166, Remove the redundant unit test function test_sample_journal_not_empty from the test module: it only asserts include_str!("fixtures/sample.journal") is non-empty (already guaranteed at compile time) and is covered by test_parse_sample_journal_fixture; delete the entire fn test_sample_journal_not_empty() { ... } block to reduce test noise.
119-139: ⚡ Quick winTighten shallow string-
containsassertions on parsed amounts.
assert!(amt.quantity.contains("47.18"))will pass for"47.18","-47.18","147.180", etc. Useassert_eq!to lock down the parser's exact serialization (sign handling and decimal preservation are specifically the things you'd want to regression-test).♻️ Tightened assertions
-#[test] -fn test_amount_left_symbol() { - let amt = parse_ok(amountp(), "$47.18"); - assert_eq!(amt.commodity, "$"); - assert!(amt.quantity.contains("47.18")); - assert_eq!(amt.style.commodity_side, CommoditySide::Left); -} +#[test] +fn test_amount_left_symbol() { + let amt = parse_ok(amountp(), "$47.18"); + assert_eq!(amt.commodity, "$"); + assert_eq!(amt.quantity, "47.18"); + assert_eq!(amt.style.commodity_side, CommoditySide::Left); +} @@ -#[test] -fn test_amount_no_symbol() { - let amt = parse_ok(amountp(), "47.18"); - assert!(amt.commodity.is_empty()); - assert!(amt.quantity.contains("47.18")); -} +#[test] +fn test_amount_no_symbol() { + let amt = parse_ok(amountp(), "47.18"); + assert!(amt.commodity.is_empty()); + assert_eq!(amt.quantity, "47.18"); + assert_eq!(amt.style.commodity_side, CommoditySide::Right); +}As per coding guidelines: "When working on tests, proactively improve them - remove useless or shallow tests and consolidate repeated setup code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/tests/mod.rs` around lines 119 - 139, The tests use loose contains() checks on amt.quantity; tighten them to assert the exact serialized quantity so sign and decimal precision are locked down: in test_amount_left_symbol, replace assert!(amt.quantity.contains("47.18")) with assert_eq!(amt.quantity, "47.18"); in test_amount_left_symbol_spaced keep checking amt.style.commodity_spaced but also assert_eq!(amt.quantity, "47.18"); and in test_amount_no_symbol replace assert!(amt.quantity.contains("47.18")) with assert_eq!(amt.quantity, "47.18"); ensure you reference the parsed amt from amountp() and the fields amt.quantity, amt.commodity, and amt.style.commodity_spaced when making these changes.
168-180: 💤 Low valueConsider a snapshot test for
test_parse_sample_journal_fixture.The fixture-based assertions check a hand-picked subset of fields (count, first/fourth txn). A snapshot of the full parsed
Vec<Transaction>would catch ordering/posting/amount drift across the whole fixture in one place and would be cheaper to maintain.As per coding guidelines: "Use snapshot tests, TDD, and TDD macros found in the repo to ensure longevity of test usefulness".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/tests/mod.rs` around lines 168 - 180, Replace the handful of field assertions in the test_parse_sample_journal_fixture unit test with (or add) a snapshot assertion that captures the full parsed Vec<Transaction> returned by parse_journal_ok; serialize txns (e.g., via Debug/Display, serde_json/serde_yaml, or the repo's TDD snapshot macro such as insta::assert_debug_snapshot!/assert_yaml_snapshot!) so the snapshot stores all Transaction fields (including postings, amounts, ordering and Status enum values) and commit the generated snapshot fixture so regressions in Transaction or parse_journal_ok are detected automatically.src/plug_dayledger/hledger/parse/journal.rs (2)
117-131: ⚡ Quick win
transactionpaccepts a header with zero postings.
posting.repeated().collect()allows an emptypostingsvec, so a stray2024/01/02 desc\n(with no following indented lines) is silently parsed as a 0-postingTransaction. That likely produces a downstream Claim with zeroposting_hintsand an emptytxn_amount_totals, which the matcher treats with the 0.5 fallback (line 352–354 inclaim_matcher.rs). Either reject as a parse error or surface it; hledger itself rejects 0-posting transactions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/parse/journal.rs` around lines 117 - 131, The transaction parser currently allows zero-posting transactions because posting.repeated().collect() can produce an empty Vec; update the parser that builds Transaction (the header.then(posting.repeated().collect::<Vec<_>>()).map(...) closure in transactionp) to reject empty postings—either replace repeated() with a combinator that requires at least one posting or, after collecting postings, check postings.is_empty() and return a parse error (propagate Err) instead of constructing a Transaction; reference the header, posting, and transactionp symbols so the change targets that mapping/closure.
6-14: 💤 Low value
default_amountcouples tois_missingvia emptyquantity.This works because
Amount::is_missingchecksquantity.is_empty(), but the contract is implicit. Consider derivingDefaultforAmount(it already requiresDefaultonAmountStyle, which is provided) and reusingAmount::default(), or expose this asAmount::missing()so the intent is self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/parse/journal.rs` around lines 6 - 14, default_amount currently relies on Amount::is_missing checking quantity.is_empty(), making the “missing” contract implicit; derive or implement Default for Amount (ensuring AmountStyle: Default is used) and replace usages of default_amount with Amount::default(), or alternatively add an explicit constructor Amount::missing() that returns the same sentinel (empty commodity/quantity, default style, None cost/cost_basis); update callers to use Amount::default() or Amount::missing() and remove the standalone fn default_amount().src/plug_dayledger/hledger/claim_matcher.rs (2)
318-347: 💤 Low valueDate heuristic ordering: tied semantics need a comment.
The cascading thresholds in
date_scoreare clever but order-sensitive in non-obvious ways (e.g., the month/day-swap check at lines 331–336 only fires when years match; the same-month-and-day check at 337 captures off-by-year-only). A short comment summarizing the pecking order — and what each branch is meant to catch — would prevent future regressions when someone adjusts thresholds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/claim_matcher.rs` around lines 318 - 347, The date_score function's cascade of thresholds is order-sensitive and lacks documentation; add a concise comment above fn date_score explaining the intended pecking order and semantics of each branch: exact match (new_date == old_date) -> 1.0, one-day difference -> 0.90, same-year but month/day swapped (the month/day-swap check that compares new_date.month() to old_date.day() and new_date.day() to old_date.month()) -> 0.82, exact month+day match (same month and day regardless of year) -> 0.78, same year & same day (different month) -> 0.70, within 7 days -> 0.62, else 0.0; note why the swap check is constrained to same year and that ordering prevents the same-month/day branch from preempting swap detection so future edits to date_score (and variables new_date, old_ts) won't inadvertently change behavior.
199-227: ⚖️ Poor tradeoffFloat equality on
*best == candidate.scoreworks here but is fragile.The "is best" check relies on bitwise float equality between
candidate.scoreand the value re-pushed into thescores_by_*lists. It works today because both originate from the samecomposite_score()invocation per (new, old) pair, but any future refactor that re-derives scores or applies any arithmetic adjustment will silently break tie-breaking.Consider keying candidates by
(usize, OrderedFloat)and selecting "best" by tracking the candidate index rather than re-comparing floats.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/claim_matcher.rs` around lines 199 - 227, The current candidates.retain logic uses exact float equality (*best == candidate.score) to detect the top scorer, which is fragile; instead change how you record the best per index: when building scores_by_new and scores_by_old (or the data structure that aggregates composite_score values), key entries by (index, OrderedFloat(score)) or store alongside each score the originating candidate id/index so you can determine "best" by comparing keys/indices rather than re-comparing f64s; update the candidate filtering to check that the candidate's id/index matches the recorded best index for candidate.new_index and candidate.old_index and keep the existing margin checks (HEURISTIC_MIN_MARGIN) unchanged.src/plug_dayledger/hledger/types.rs (1)
24-34: 💤 Low value
DateDisplay does not handle negative years correctly.
{:04}doesn't apply zero-padding to negative numbers in the way the format suggests (e.g., year-1renders as"-1", not"-001"or"-0001"). hledger journals don't typically carry BC dates, so this is unlikely to bite, but worth a passing thought if you ever round-trip serialized dates through thisDisplay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/types.rs` around lines 24 - 34, The Display impl for Date (impl std::fmt::Display for Date, fn fmt) needs to handle negative years explicitly because "{:04}" won't produce a zero-padded negative year (e.g., year -1 should render "-0001"); change the formatting logic to compute the year value first (let year = self.0.year()), take the absolute value for zero-padding, and then write a prefixed '-' when year is negative (use the same write! in fmt but format abs(year) with {:04} and prepend '-' for negative years) so both positive and negative years produce the expected zero-padded output.src/plug_dayledger/types.rs (2)
88-95: ⚡ Quick win
ClaimPostingHint.hint_type: Option<String>would benefit from a typed enum.
hint_typeis populated with stringly-typed values ("virtual","balancedVirtual") byclaim_matcher::build_claimand compared by string inclaim_posting_pairs. An enum (mirroring or reusingPostingType) eliminates the typo risk and lets the JSON shape be governed by serde rather than ad-hocformat!.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/types.rs` around lines 88 - 95, Change ClaimPostingHint.hint_type from Option<String> to a typed enum (e.g., Option<PostingType> or a new enum like ClaimHintType) and update serialization to derive Serialize/Deserialize so JSON ("virtual", "balancedVirtual") is handled by serde; update claim_matcher::build_claim to construct the enum variant instead of formatting strings and update claim_posting_pairs to match on the enum variants rather than comparing strings (or implement TryFrom<String>/FromStr for the enum and use that conversion where strings are currently produced/consumed). Ensure the enum derives Debug, Clone, Serialize, Deserialize, PartialEq (and schemars::JsonSchema if needed) and adjust any places constructing or comparing hint_type accordingly.
99-113: ⚡ Quick win
HledgerTxnDeets.tagsasVec<(String, String)>serializes as nested arrays.Anonymous tuples serialize to JSON as
[["key","value"], ...], which is awkward for downstream consumers (camelCase rename has no effect, and ad-hoc tools can't address fields by name). Consider either a small namedTag { key, value }struct orBTreeMap<String, Vec<String>>if tags are key→multi-value.Note this is the persistent JSON shape stored in claim deets, so changing it later requires migration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/types.rs` around lines 99 - 113, HledgerTxnDeets.tags currently uses Vec<(String,String)>, which serializes to nested arrays; change it to a named serializable type (e.g., define a Tag struct with pub key: String and pub value: String and make tags: Vec<Tag>) or to a map type (e.g., BTreeMap<String,Vec<String>>) depending on whether you need multi-values; update the HledgerTxnDeets struct (tags field) and any code that constructs/parses tags (search for HledgerTxnDeets and usages) so serialization produces objects/keys instead of arrays, and ensure serde attributes (if any) reflect the chosen shape and handle migration of the persisted JSON accordingly.src/plug_dayledger/hledger/parse/common.rs (1)
142-207: 💤 Low value
amountpright-side branch setscommodity_spaced = trueunconditionally.In the
right_or_no_symbolarm, when anopt_commodityis present it always buildsAmountStyle { commodity_spaced: true, ... }. That's becausews1().ignore_then(commoditysymbolp())requires whitespace, so today it's always preceded by a space — but the field is named to round-trip styling, and a future relaxation of the whitespace requirement (e.g.,47.18USD) would silently misreport spacing. Consider capturing whether the leading whitespace existed (ws1().or_not()mapped to a bool) so the style flag tracks the source faithfully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_dayledger/hledger/parse/common.rs` around lines 142 - 207, In amountp, the right_or_no_symbol branch currently sets AmountStyle.commodity_spaced = true whenever opt_commodity is Some because ws1().ignore_then(commoditysymbolp()) forces whitespace; change the parser to capture whether whitespace was present (e.g. replace ws1().ignore_then(commoditysymbolp()).or_not() with ws1().map(|_| true).or(empty().to(false)).then(commoditysymbolp()).or_not() or equivalent) so the mapped closure receives a bool for spacing plus the commodity; then use that bool to set commodity_spaced instead of hardcoding true (update the mapping in right_or_no_symbol that constructs Amount and refers to opt_commodity and decimal_mark).src/daybook_ffi/rt.rs (1)
85-111: ⚖️ Poor tradeoffConsider renaming
dispatch_doc_facet→dispatch_doc_routinefor clarity.The function name still references
DocFacetwhile it now dispatchesDocRoutine. Since this is on the FFI surface, the rename is a breaking change for callers (UniFFI bindings consumers); defer if the FFI consumers haven't been migrated yet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_ffi/rt.rs` around lines 85 - 111, Rename the FFI method dispatch_doc_facet to dispatch_doc_routine to reflect that it dispatches DispatchArgs::DocRoutine; update the function signature name (dispatch_doc_facet → dispatch_doc_routine) and any public/exported references (including UniFFI binding metadata) so callers see the clearer name, and if you need to preserve backward compatibility keep a thin shim function named dispatch_doc_facet that forwards to dispatch_doc_routine (preserving the same params and return type) while marking it deprecated so external consumers can migrate.src/plug_test/e2e/capability_regression_wflow.rs (2)
25-25: 💤 Low valueTake
&strinstead of&Stringfordoc_id.Idiomatic Rust prefers
&strparameters;&Stringforces callers to hold aString. Same applies to other helpers in the file.♻️ Suggested change
-async fn dispatch_and_wait( - test_cx: &daybook_core::test_support::DaybookTestContext, - routine_name: &str, - doc_id: &String, - changed_facet_keys: Vec<String>, -) -> Res<String> { +async fn dispatch_and_wait( + test_cx: &daybook_core::test_support::DaybookTestContext, + routine_name: &str, + doc_id: &str, + changed_facet_keys: Vec<String>, +) -> Res<String> {Update the
doc_id.clone()inDispatchArgs::DocRoutinetodoc_id.to_string()and call sites accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_test/e2e/capability_regression_wflow.rs` at line 25, Change the doc_id parameter types from &String to &str across this file (e.g., the function that currently declares doc_id: &String), update any places constructing DispatchArgs::DocRoutine to call doc_id.to_string() instead of doc_id.clone(), and update all call sites and helper functions in this file to accept &str (or call .to_string() when an owned String is needed); ensure signatures, parameter usages, and any pattern matches are adjusted to use &str so callers are not forced to hold a String.
22-70: ⚡ Quick winConsolidate repeated test setup/assertion patterns.
The eight
#[tokio::test]s repeat a near-identical preamble (test_cx,import_test_plug_oci,setup_doc,dispatch_and_wait,open_plug_test_local_state,fetch_capability_report*) and the two "config tag rights READ+UPDATE / READ-only" assertion blocks (lines 190-209 and 300-319 are duplicated). Extracting a small helper (assert_config_tag_rights(&config_tag_keys, &config_tag_rights)) and a setup helper would shrink each test to its unique assertions and reduce drift risk.Per coding guidelines: "When working on tests, proactively improve them ... consolidate repeated setup code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_test/e2e/capability_regression_wflow.rs` around lines 22 - 70, Extract the repeated test preamble and duplicated assertions into small helpers: create a setup helper (e.g., setup_plug_test) that performs the sequence currently repeated (import_test_plug_oci, setup_doc, calling dispatch_and_wait, open_plug_test_local_state, and invoking the various fetch_capability_report* functions) and return the values each test needs; and create an assert_config_tag_rights(&config_tag_keys, &config_tag_rights) helper that contains the two duplicated "config tag rights READ+UPDATE / READ-only" assertion blocks so tests call this single helper instead of duplicating logic. Update each #[tokio::test] to call setup_plug_test and assert_config_tag_rights and keep only test-unique assertions; reference the existing dispatch_and_wait function and open_plug_test_local_state and fetch_capability_report* calls when moving logic into the new helpers.src/daybook_core/wit/main.wit (2)
67-70: 💤 Low valueConsider clarifying the semantics of
doc-meta::updated-at.Modeling
updated-ataslist<datetime>is unusual; most consumers expect a single most-recent timestamp. If the intent is a history of updates (e.g., per-branch or per-CRDT change), an inline doc comment in the WIT would help guests interpret the field correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/wit/main.wit` around lines 67 - 70, doc-meta::updated-at is defined as list<datetime> which is ambiguous; either change its type to a single datetime if you only need the latest timestamp, or keep list<datetime> but add an inline WIT doc comment on the record doc-meta (and specifically updated-at) explaining the semantics (e.g., chronological history of updates, per-branch timestamps, CRDT change history) so guests know whether to read the last element as the current value or treat the list as a full audit trail; update the record doc-meta declaration and its updated-at field comment accordingly.
85-93: 💤 Low value
doc-token::clonewithnonedownscope returns full rights — add WIT doc comment to clarify.When
downscopeisnone, the implementation returns a token with the same rights as the source. This appears intentional (supporting unrestricted copy operations), but the WIT interface lacks documentation explaining this semantics. Add a doc comment to clarify whethernonemeans "preserve all rights" or "no restriction" so guests understand the behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/wit/main.wit` around lines 85 - 93, Add a WIT doc comment to the doc-token::clone method clarifying the semantics of the downscope parameter: state that passing none returns a token that preserves the original token's full rights (i.e., no restriction), whereas passing Some(downscope) returns a token restricted to the supplied doc-rights; place this comment immediately above the clone signature in the resource doc-token so callers understand that none means "preserve all rights / no restriction."src/plug_test/e2e/common.rs (1)
8-15: 💤 Low valueHardcoded
../../target/ociignoresCARGO_TARGET_DIRoverride.If a developer or CI sets
CARGO_TARGET_DIR(or uses[build] target-dirin.cargo/config.toml), this path won't resolve and the test will fail with the misleading "build it first" hint. Consider honoring the env var or accepting a path override.♻️ Optional fix
- let artifact_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join("../../target/oci") - .join(crate::plug_manifest().id()); + let target_dir = std::env::var_os("CARGO_TARGET_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../target") + }); + let artifact_path = target_dir.join("oci").join(crate::plug_manifest().id());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_test/e2e/common.rs` around lines 8 - 15, The artifact_path logic in common.rs currently hardcodes "../../target/oci" and ignores CARGO_TARGET_DIR; change it to first check std::env::var("CARGO_TARGET_DIR") and if present use PathBuf::from(that_value).join("oci").join(crate::plug_manifest().id()), otherwise fall back to the original env!("CARGO_MANIFEST_DIR")..../../target/oci behavior; update the artifact_path construction and the eyre::ensure error message to reflect the actual path checked so failures show the correct location.src/plug_plabels/wflows/label_note.rs (1)
49-58: ⚡ Quick win
rw_config_tokenandro_config_tokenmay resolve to the same token, leaving the fallback pattern inefficient.When a facet token holds both
READandUPDATErights, both.find()calls will return it, makingro_config_tokenredundant. The fallback logic (if let Some(rw) ... else if let Some(ro) ...) means thero_config_tokenbranch becomes unreachable whenever both resolve to the same token. This is safe but wasteful.To clarify intent—prefer a write-capable token when available, otherwise fall back to a strictly read-only token—consider filtering
ro_config_tokento exclude tokens withUPDATErights:Suggested approach
- let rw_config_token = args - .config_docs - .iter() - .flat_map(|cd| cd.facets.iter()) - .find(|t| t.key() == config_facet_key && t.rights().contains(FacetRights::UPDATE)); - let ro_config_token = args - .config_docs - .iter() - .flat_map(|cd| cd.facets.iter()) - .find(|t| t.key() == config_facet_key && t.rights().contains(FacetRights::READ)); + let rw_config_token = args + .config_docs + .iter() + .flat_map(|cd| cd.facets.iter()) + .find(|t| t.key() == config_facet_key && t.rights().contains(FacetRights::UPDATE)); + // Prefer a strictly read-only token over one with UPDATE rights for the fallback. + let ro_config_token = args + .config_docs + .iter() + .flat_map(|cd| cd.facets.iter()) + .filter(|t| t.key() == config_facet_key && t.rights().contains(FacetRights::READ)) + .find(|t| !t.rights().contains(FacetRights::UPDATE));This pattern repeats identically in
label_image.rs,learn_note_label_candidates.rs, andlearn_image_label_candidates.rsand could be refactored consistently across all four files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_plabels/wflows/label_note.rs` around lines 49 - 58, rw_config_token and ro_config_token can end up identical when a facet has both READ and UPDATE, making the read fallback redundant; change the selection so you prefer a token with UPDATE rights and only fall back to a token that has READ but not UPDATE. Concretely, keep the existing rw_config_token search (find facet where key==config_facet_key and rights contains FacetRights::UPDATE), then change the ro_config_token search to exclude UPDATE (find facet where key==config_facet_key and rights contains FacetRights::READ and NOT contains FacetRights::UPDATE) so the fallback branch is meaningful; apply the same change consistently in label_image.rs, learn_note_label_candidates.rs, and learn_image_label_candidates.rs.src/daybook_core/rt/dispatch.rs (1)
102-120: 💤 Low valueConsider migration impact on persisted dispatch rows.
The new fields
invocation,primary_doc,config_docslack#[serde(default)]. If any sqlite store has existingpayload_jsonrows persisted before this change (e.g., a long-running dev DB),load_state(line 861) will fail during startup with a deserialization error. If you do not intend to support that migration path, it's fine to leave as-is, but consider either adding#[serde(default)]or documenting that the dispatch sqlite must be recreated for this version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/rt/dispatch.rs` around lines 102 - 120, The FacetRoutineArgs struct now introduces non-optional fields (invocation, primary_doc, config_docs) which will break deserialization of existing persisted payload_json rows; to fix, add #[serde(default)] to the invocation, primary_doc and config_docs fields in the FacetRoutineArgs definition so deserialization of older rows falls back to defaults (or alternatively document/reject the migration path), ensuring you reference the FacetRoutineArgs type and those field names when making the change so load_state can read old rows without failing.src/daybook_core/rt/wash_plugin.rs (1)
786-838: 💤 Low valueOwner-plug-id resolution depends on the first ACL entry being representative.
Line 789-793 derives
owner_plug_idfromconfig_doc_meta.facet_acl.first(). This is correct only if every entry in a singleconfig_doc_meta.facet_aclshares the sameowner_plug_id(which the dispatch grouping should guarantee, but isn't enforced here). If a future change accidentally mixes owners in one entry, the loop would silently group everything under the first entry's owner. Consider asserting consistency or grouping explicitly per owner during construction at the dispatch site rather than per-first-entry here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daybook_core/rt/wash_plugin.rs` around lines 786 - 838, The code currently picks owner_plug_id from config_doc_meta.facet_acl.first(), which can silently mis-group facets if ACL entries have differing owner_plug_id; update the loop that processes config_docs (symbols: config_doc_meta, facet_acl, owner_plug_id, owner_config_docs, get_or_init_plug_config_doc_id, build_doc_facet_tokens) to validate/assert that every entry in config_doc_meta.facet_acl yields the same owner_plug_id (or explicitly split the config_doc_meta by owner_plug_id and process each group separately), and return an error if inconsistent; this ensures owner resolution is deterministic and prevents accidental cross-owner grouping.src/plug_plabels/wflows/learn_note_label_candidates.rs (1)
52-54: ⚡ Quick winSilent no-ops when only RO config token is available may hide regressions.
Two paths silently discard work:
- Line 52-54: if neither
rw_config_tokennorro_config_tokenis present, the routine returnsOk(())with no log. Whether this is reachable depends on the dispatch ACL, but atracing::debug!would help diagnose missing config wiring.- Lines 93-110: when
merged != proposal_setbut onlyro_config_tokenexists, the freshly-computed merge is computed (including LLM call and embedding centroid lookups on the prior lines) and then dropped silently. At minimum log adebug!/warn!so operators can spot this state if it ever occurs.Also applies to: 93-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plug_plabels/wflows/learn_note_label_candidates.rs` around lines 52 - 54, The function currently returns early with Ok(()) when both rw_config_token and ro_config_token are None and silently drops a computed merge when merged != proposal_set but only ro_config_token is present; add tracing logs in both places: (1) before the early return involving rw_config_token and ro_config_token, emit tracing::debug! including the presence/absence of each token and any request/dispatch identifiers to help diagnose missing config wiring; (2) before discarding the freshly-computed merged (the branch where merged != proposal_set and rw_config_token.is_none() but ro_config_token.is_some()), emit a tracing::warn! or tracing::debug! with context such as proposal_set id/length and merged id/length and note that the merge is being skipped due to read-only config so operators can spot this unexpected state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6aaa902-8362-41cb-9222-175eb1a4aa8e
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockand included by**/*docs/DEVDOC/log.mdis excluded by!docs/DEVDOC/**and included by**/*docs/DEVDOC/todo.mdis excluded by!docs/DEVDOC/**and included by**/*
📒 Files selected for processing (80)
CONTRIBUTING.mdCargo.tomlflake.nixsrc/am_utils_rs/repo/iroh.rssrc/am_utils_rs/sync.rssrc/daybook_cli/main.rssrc/daybook_core/e2e/command_invoke_wflow.rssrc/daybook_core/e2e/embed_text_wflow.rssrc/daybook_core/e2e/plugin_local_index_wflow.rssrc/daybook_core/plugs.rssrc/daybook_core/rt.rssrc/daybook_core/rt/dispatch.rssrc/daybook_core/rt/triage.rssrc/daybook_core/rt/wash_plugin.rssrc/daybook_core/rt/wash_plugin/caps.rssrc/daybook_core/rt/wash_plugin/mltools.rssrc/daybook_core/wit/main.witsrc/daybook_ffi/rt.rssrc/daybook_types/Cargo.tomlsrc/daybook_types/doc.rssrc/daybook_types/manifest.rssrc/daybook_types/reference.rssrc/daybook_wflows/lib.rssrc/daybook_wflows/wflows/embed_image.rssrc/daybook_wflows/wflows/embed_text.rssrc/daybook_wflows/wflows/index_embedding.rssrc/daybook_wflows/wflows/ocr_image.rssrc/daybook_wflows/wflows/test_labeler.rssrc/mltools/lib.rssrc/plug_dayledger/Cargo.tomlsrc/plug_dayledger/e2e.rssrc/plug_dayledger/e2e/common.rssrc/plug_dayledger/e2e/parse_hledger_wflow.rssrc/plug_dayledger/hledger/claim_matcher.rssrc/plug_dayledger/hledger/mod.rssrc/plug_dayledger/hledger/parse/common.rssrc/plug_dayledger/hledger/parse/journal.rssrc/plug_dayledger/hledger/parse/mod.rssrc/plug_dayledger/hledger/tests/fixtures/mod.rssrc/plug_dayledger/hledger/tests/fixtures/quickstart.journalsrc/plug_dayledger/hledger/tests/fixtures/sample.journalsrc/plug_dayledger/hledger/tests/fixtures/sample2.journalsrc/plug_dayledger/hledger/tests/fixtures/status.journalsrc/plug_dayledger/hledger/tests/fixtures/unicode.journalsrc/plug_dayledger/hledger/tests/mod.rssrc/plug_dayledger/hledger/types.rssrc/plug_dayledger/lib.rssrc/plug_dayledger/types.rssrc/plug_dayledger/wflows/mod.rssrc/plug_dayledger/wflows/parse_hledger.rssrc/plug_dayledger/wit/deps/api-utilssrc/plug_dayledger/wit/deps/daybooksrc/plug_dayledger/wit/deps/daybook-typessrc/plug_dayledger/wit/deps/mltoolssrc/plug_dayledger/wit/deps/townframe-sqlsrc/plug_dayledger/wit/deps/utilssrc/plug_dayledger/wit/deps/wasi-cli-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasi-clocks-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasi-config-0.2.0-draft/package.witsrc/plug_dayledger/wit/deps/wasi-filesystem-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasi-http-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasi-io-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasi-keyvalue-0.2.0-draft/package.witsrc/plug_dayledger/wit/deps/wasi-logging-0.1.0-draft/package.witsrc/plug_dayledger/wit/deps/wasi-random-0.2.6/package.witsrc/plug_dayledger/wit/deps/wasmcloud-postgres-0.1.1-draft/package.witsrc/plug_dayledger/wit/deps/wflowsrc/plug_dayledger/wit/main.witsrc/plug_plabels/lib.rssrc/plug_plabels/wflows/label_engine.rssrc/plug_plabels/wflows/label_image.rssrc/plug_plabels/wflows/label_note.rssrc/plug_plabels/wflows/learn_image_label_candidates.rssrc/plug_plabels/wflows/learn_note_label_candidates.rssrc/plug_test/Cargo.tomlsrc/plug_test/e2e.rssrc/plug_test/e2e/capability_regression_wflow.rssrc/plug_test/e2e/common.rssrc/plug_test/lib.rsx/build-a-dayb.ts
💤 Files with no reviewable changes (2)
- src/daybook_wflows/lib.rs
- Cargo.toml
| doc_acls: vec![RoutineDocAcl { | ||
| doc_predicate: DocPredicateClause::HasTag( | ||
| WellKnownFacetTag::Note.into(), | ||
| ), | ||
| facet_acl: vec![ | ||
| RoutineFacetAccess { | ||
| owner_plug_id: None, | ||
| tag: WellKnownFacetTag::Blob.into(), | ||
| key_id: None, | ||
| read: true, | ||
| write: false, | ||
| create: false, | ||
| delete: false, | ||
| }, | ||
| RoutineFacetAccess { | ||
| owner_plug_id: None, | ||
| tag: WellKnownFacetTag::Note.into(), | ||
| key_id: None, | ||
| read: true, | ||
| write: true, | ||
| create: false, | ||
| delete: false, | ||
| }, | ||
| ], | ||
| config_facet_acl: vec![], | ||
| }, | ||
| }], | ||
| query_acls: vec![], | ||
| config_facet_acl: vec![], |
There was a problem hiding this comment.
These routine ACLs no longer match the docs their processors run on.
ocr-image, embed-image, embed-text, and test-label are triggered on docs that are missing the output facet, but these doc_acls require that output tag and only grant write instead of create. Under the new token model that means the workflow won't receive a tag/create token for the first write, so initial OCR/embedding/label creation will fail. Make the ACL predicate match the trigger doc and grant create on the produced facet tag.
Also applies to: 210-237, 249-276, 320-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daybook_core/plugs.rs` around lines 171 - 197, Update each RoutineDocAcl
so the doc_predicate matches the trigger document tag (the tag the processor
watches) and ensure the RoutineFacetAccess entry for the facet the processor
produces sets create: true (not just write). Specifically, for the RoutineDocAcl
instances around the shown diff (and the other ranges mentioned) change the
DocPredicateClause to the trigger tag used by the processor and set the produced
facet's RoutineFacetAccess.create = true (leave read/write as appropriate);
verify the owner_plug_id/tag/key_id entries stay the same. This ensures the
workflow receives a create token for the initial OCR/embedding/label write.
| } if key == "vtag" | ||
| && patch.path.len() == 3 | ||
| && patch.path[1].1 == automerge::Prop::Map("manifests".into()) => | ||
| { | ||
| if key == "vtag" { | ||
| let Some((_obj, automerge::Prop::Map(plug_id))) = patch.path.get(2) else { | ||
| return Ok(()); | ||
| }; | ||
| let Some((_obj, automerge::Prop::Map(plug_id))) = patch.path.get(2) else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let vtag_bytes = match val { | ||
| automerge::Value::Scalar(scalar) => match &**scalar { | ||
| automerge::ScalarValue::Bytes(bytes) => bytes, | ||
| _ => return Ok(()), | ||
| }, | ||
| let vtag_bytes = match val { | ||
| automerge::Value::Scalar(scalar) => match &**scalar { | ||
| automerge::ScalarValue::Bytes(bytes) => bytes, | ||
| _ => return Ok(()), | ||
| }; | ||
| let vtag = VersionTag::hydrate_bytes(vtag_bytes)?; | ||
| let event_origin = crate::repos::resolve_origin_from_vtag_actor( | ||
| &self.local_actor_id, | ||
| &vtag.actor_id, | ||
| live_origin, | ||
| ); | ||
| if vtag.version.is_nil() { | ||
| out.push(PlugsEvent::PlugAdded { | ||
| id: plug_id.clone(), | ||
| heads: heads.clone(), | ||
| origin: event_origin.clone(), | ||
| }); | ||
| } else { | ||
| out.push(PlugsEvent::PlugChanged { | ||
| id: plug_id.clone(), | ||
| heads: heads.clone(), | ||
| origin: event_origin.clone(), | ||
| }); | ||
| } | ||
| }, | ||
| _ => return Ok(()), | ||
| }; | ||
| let vtag = VersionTag::hydrate_bytes(vtag_bytes)?; |
There was a problem hiding this comment.
Don't treat malformed manifest vtag patches as a no-op.
Returning Ok(()) here silently drops unexpected Automerge shapes, which can leave the in-memory plug index stale relative to replicated state. Since this path consumes shared external data, surface drift/corruption as an error instead of ignoring it.
Based on learnings: when validating data that comes from external inputs (such as automerge documents and related metadata like dmeta), use eyre::bail!() to return errors rather than panicking or silently recovering so version drift is surfaced safely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daybook_core/plugs.rs` around lines 1082 - 1097, The current
patch-handling branch in plugs.rs silently returns Ok(()) for malformed `vtag`
shapes (the match arm that checks key == "vtag" and patch.path len==3 and
path[1] map "manifests"), which can hide replica drift; instead of returning
Ok(()), replace those silent early returns with eyre::bail! calls that surface
an error with context (include identifying info like the patch.key, patch.path
or plug_id when available). Specifically, for the `let Some((_obj,
automerge::Prop::Map(plug_id)))` failure, for non-Bytes scalar, and for
non-Scalar automerge::Value cases, use eyre::bail! with a clear message
indicating a malformed manifest vtag for plug_id (or full path if plug_id
missing); keep VersionTag::hydrate_bytes(...) usage but propagate its error via
eyre as well.
| let DispatchArgs::DocRoutine { | ||
| doc_id, | ||
| branch_path, | ||
| heads, | ||
| changed_facet_keys, | ||
| wflow_args_json, | ||
| } = args; | ||
|
|
||
| let invocation = if changed_facet_keys.is_empty() { | ||
| dispatch::RoutineInvocation::Command | ||
| } else { | ||
| dispatch::RoutineInvocation::Processor(dispatch::ProcessorInvocation { | ||
| trigger_doc_id: doc_id.clone(), | ||
| changed_facet_keys, | ||
| }) | ||
| }; |
There was a problem hiding this comment.
Don't infer processor-vs-command from changed_facet_keys.is_empty().
Processor-triggered runs can legitimately arrive with no changed keys, so this heuristic rewrites them into RoutineInvocation::Command and drops the processor context. The invocation kind needs to be carried explicitly in DispatchArgs::DocRoutine instead of inferred here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daybook_core/rt.rs` around lines 1601 - 1616, Dispatch currently infers
RoutineInvocation kind from changed_facet_keys.is_empty(), which wrongly
converts processor-triggered runs with no keys into Command; update
DispatchArgs::DocRoutine to include an explicit invocation kind (e.g. an enum
field like invocation_kind or routine_kind) and change this construction to use
that field to choose between dispatch::RoutineInvocation::Command and
dispatch::RoutineInvocation::Processor(dispatch::ProcessorInvocation {
trigger_doc_id: doc_id.clone(), changed_facet_keys }), rather than relying on
changed_facet_keys.is_empty(); ensure all callers/builders of
DispatchArgs::DocRoutine are updated to provide the new explicit kind.
| let dispatch_id = { | ||
| let mut identity = String::new(); | ||
| use std::fmt::Write as _; | ||
| write!( | ||
| &mut identity, | ||
| "{}|{}|{}|{}", | ||
| doc_id, | ||
| am_utils_rs::serialize_commit_heads(heads.as_ref()).join(","), | ||
| plug_id, | ||
| routine_name | ||
| ) | ||
| .expect("writing to string should never fail"); | ||
| utils_rs::hash::blake3_hash_bytes(identity.as_bytes()) | ||
| }; | ||
| let mut dispatch_id = fixed_dispatch_id | ||
| .clone() | ||
| .unwrap_or_else(|| format!("{plug_id}/{routine_name}-{dispatch_id}")); | ||
|
|
There was a problem hiding this comment.
Include branch_path in the dispatch identity.
The new fingerprint no longer distinguishes branches. If the same processor is queued for two branches that share the same heads, one branch will reuse or suppress the other dispatch even though triage_doc treated them as separate work items.
Suggested fix
let dispatch_id = {
let mut identity = String::new();
use std::fmt::Write as _;
write!(
&mut identity,
- "{}|{}|{}|{}",
+ "{}|{}|{}|{}|{}",
doc_id,
+ branch_path.as_str(),
am_utils_rs::serialize_commit_heads(heads.as_ref()).join(","),
plug_id,
routine_name
)
.expect("writing to string should never fail");
utils_rs::hash::blake3_hash_bytes(identity.as_bytes())
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daybook_core/rt.rs` around lines 1618 - 1635, The dispatch identity
currently built in the block that creates dispatch_id (using identity string,
serialize_commit_heads, and blake3_hash_bytes) omits branch_path so different
branches with identical heads collide; update the identity construction inside
the let dispatch_id = { ... } block to include branch_path in the formatted
string (alongside doc_id, serialized heads, plug_id, routine_name) and ensure
the fallback fixed_dispatch_id unwrap_or_else string (where
format!("{plug_id}/{routine_name}-{dispatch_id}")) also incorporates branch_path
so the final dispatch key uniquely includes branch_path; adjust references to
dispatch_id, fixed_dispatch_id, serialize_commit_heads, and blake3_hash_bytes
accordingly.
| let changed_facet_keys: Vec<String> = changed_facet_keys | ||
| .map(|keys| { | ||
| keys.iter() | ||
| .filter(|key| { | ||
| processor.read_tags.contains(&key.tag.to_string()) | ||
| || processor.read_keys.contains(key) | ||
| }) | ||
| .map(|key| key.to_string()) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); | ||
| let args = DispatchArgs::DocRoutine { | ||
| doc_id: doc_id.clone(), | ||
| branch_path: branch_path.clone(), | ||
| heads: doc_heads.clone(), | ||
| changed_facet_keys, | ||
| wflow_args_json: None, | ||
| }; |
There was a problem hiding this comment.
Added/removed-only processor events lose their trigger facet keys.
This payload is built only from changed_facet_keys. Processors that matched via added_facet_keys or removed_facet_keys therefore dispatch with an empty list, so the routine loses the facet context that actually triggered it.
Suggested fix
- let changed_facet_keys: Vec<String> = changed_facet_keys
- .map(|keys| {
- keys.iter()
- .filter(|key| {
- processor.read_tags.contains(&key.tag.to_string())
- || processor.read_keys.contains(key)
- })
- .map(|key| key.to_string())
- .collect()
- })
- .unwrap_or_default();
+ let changed_facet_keys: Vec<String> = changed_facet_keys
+ .into_iter()
+ .flat_map(|keys| keys.iter())
+ .chain(added_facet_keys.into_iter().flat_map(|keys| keys.iter()))
+ .chain(removed_facet_keys.into_iter().flat_map(|keys| keys.iter()))
+ .filter(|key| {
+ processor.read_tags.contains(&key.tag.to_string())
+ || processor.read_keys.contains(key)
+ })
+ .map(|key| key.to_string())
+ .collect::<std::collections::BTreeSet<_>>()
+ .into_iter()
+ .collect();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/daybook_core/rt/triage.rs` around lines 260 - 277, The dispatch payload
is built only from changed_facet_keys so processors that matched via
added_facet_keys or removed_facet_keys lose their trigger facet context; update
the creation of changed_facet_keys (used when constructing
DispatchArgs::DocRoutine) to also include any facet keys from
processor.added_facet_keys and processor.removed_facet_keys that pass the same
read filtering (processor.read_tags.contains or processor.read_keys.contains),
deduplicate and map them to strings, and then use that combined list when
building the DispatchArgs::DocRoutine so routines receive the actual triggering
facet keys.
| processors: [( | ||
| "parse-hledger".into(), | ||
| Arc::new(ProcessorManifest { | ||
| desc: "Parse hledger journal notes into dayledger claims".into(), | ||
| deets: ProcessorDeets::DocProcessor { | ||
| event_predicate: Default::default(), | ||
| routine_name: "parse-hledger".into(), | ||
| predicate: DocPredicateClause::And(vec![ | ||
| DocPredicateClause::HasTag(note_tag.clone()), | ||
| DocPredicateClause::FacetFieldMatch { | ||
| tag: note_tag, | ||
| json_path: "$.mime".into(), | ||
| operator: CompareOp::Eq, | ||
| value: serde_json::json!("text/x-hledger-journal"), | ||
| }, | ||
| ]), |
There was a problem hiding this comment.
Scope parse-hledger to note changes, not every matching doc update.
event_predicate: Default::default() plus a predicate that stays true after claims are written means every claim write from parse_hledger can enqueue the same processor again on the same doc. That creates self-induced churn and can loop if identical claim writes still advance heads. Restrict the event predicate to Note mutations, or explicitly ignore claim-only updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plug_dayledger/lib.rs` around lines 192 - 207, The DocProcessor for
"parse-hledger" is currently using event_predicate: Default::default(), causing
it to re-fire on any doc change (including claim writes it produces); update the
ProcessorManifest/ProcessorDeets for "parse-hledger" to use an event_predicate
that only matches note mutations (e.g., NoteCreated/NoteUpdated or a
MutationKind filter that requires changes to the Note body) or explicitly ignore
claim-only updates so writes emitted by parse-hledger won't re-enqueue the same
processor; locate the ProcessorManifest with routine_name "parse-hledger" and
replace the default event_predicate with a predicate that scopes to note changes
(referencing event_predicate, ProcessorDeets::DocProcessor, and note_tag).
| world imports { | ||
| import logging; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
wasi logging 0.1.0-draft WIT package.wit world imports block official definition
💡 Result:
For wasi logging 0.1.0-draft, the authoritative “package.wit” content is the WIT interface wasi:logging/logging; it defines no “world imports { … }” block. Therefore there is no official world-imports block to extract for this package. 1) Official definition (the WIT interface) The wasi-logging repository’s official WIT file defines an interface logging with: - enum level { trace, debug, info, warn, error, critical } - log: func(level: level, context: string, message: string) [1] 2) Why there’s no “world imports { … }” inside this package A “world { … }” is what declares a component’s imports/exports; the standalone file wit/logging.wit is just an interface contract (it does not declare a world). This matches how other docs describe wasi:logging/logging@0.1.0-draft: it lists the interface types/functions (level, log) rather than a world. [1][2] 3) Where “world imports” blocks show up instead Other WIT packages define worlds that import wasi:logging/logging. For example, wasmCloud’s wasi-observe imports world includes an “interface wasi:logging/logging” import in its own world-imports definition; that block is defined in the importing package, not in wasi-logging itself. [3] If you tell me the exact tooling command/path you’re using to produce “package.wit” (some tools assemble/rename files), I can map that to the corresponding upstream interface/world files in the wasi-logging repo. [1]
Citations:
- 1: https://github.com/WebAssembly/wasi-logging/blob/3293e84de91a1ead98a1b4362f95ac8af5a16ddd/wit/logging.wit
- 2: https://wasmcloud.com/docs/reference/wasi/logging
- 3: https://github.com/WebAssembly/wasi-observe/blob/main/imports.md
🏁 Script executed:
#!/bin/bash
# Check whether other vendored dep packages in this PR also contain 'world' declarations,
# to understand if this is intentional/consistent.
rg -rn '^world ' src/plug_dayledger/wit/deps/Repository: dman-os/townframe
Length of output: 646
Remove world declarations from vendored WASI dep packages.
The official wasi:logging@0.1.0-draft specification does not include a world imports block; it defines only the interface contract (enum and log function). The world imports block at lines 34–36 does not match the upstream definition and does not belong in a vendored dependency package.
This issue is not isolated: all vendored WASI packages in this repository (wasi-keyvalue, wasi-http, wasi-filesystem, wasi-clocks, wasi-config) contain similar world declarations. Per WIT conventions, world blocks should be defined in consuming component packages, not in vendored deps. Some WIT toolchains may fail to resolve or compose correctly when dep packages declare their own worlds.
Either remove these world blocks to align with the official WASI specs and conventions, or document why your toolchain requires them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plug_dayledger/wit/deps/wasi-logging-0.1.0-draft/package.wit` around
lines 34 - 36, Remove the erroneous "world imports { import logging; }" block
from the vendored package.wit (wasi-logging-0.1.0-draft) so the file only
defines the logging interface (enums and log function) and no world; update any
other vendored WASI packages (wasi-keyvalue, wasi-http, wasi-filesystem,
wasi-clocks, wasi-config) the same way by deleting their world declarations, or
alternatively add a short comment documenting why a world is required if your
toolchain truly needs it — changes should target the package.wit files and the
"world imports" block referencing the logging symbol so the vendored specs match
the upstream WASI definitions.
| record time-tz { | ||
| timesonze: string, | ||
| time: time, | ||
| } |
There was a problem hiding this comment.
Fix public field typo in time-tz record
timesonze is likely a typo for timezone. In a WIT contract this becomes part of generated bindings, so this will propagate a broken field name across all consumers.
Proposed diff
record time-tz {
- timesonze: string,
+ timezone: string,
time: time,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| record time-tz { | |
| timesonze: string, | |
| time: time, | |
| } | |
| record time-tz { | |
| timezone: string, | |
| time: time, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plug_dayledger/wit/deps/wasmcloud-postgres-0.1.1-draft/package.wit`
around lines 123 - 126, The record time-tz declares a public field named
timesonze which is a typo; rename the field to timezone in the package.wit
record time-tz so generated bindings use the correct field name, then regenerate
any bindings/stubs (and update any consumers referencing time-tz.timesonze to
time-tz.timezone) to ensure the corrected field propagates across the codebase.
| fn load_or_init_proposal_set( | ||
| rw_config_token: Option<&crate::wit::townframe::daybook::capabilities::FacetTokenRw>, | ||
| ro_config_token: Option<&crate::wit::townframe::daybook::capabilities::FacetTokenRo>, | ||
| rw_config_token: Option<&crate::wit::townframe::daybook::capabilities::FacetToken>, | ||
| ro_config_token: Option<&crate::wit::townframe::daybook::capabilities::FacetToken>, | ||
| ) -> Result<PseudoLabelCandidatesFacet, JobErrorX> { | ||
| if let Some(token) = rw_config_token { | ||
| if token.exists() { | ||
| let raw = token.get(); | ||
| let facet_raw: daybook_types::doc::FacetRaw = | ||
| serde_json::from_str(&raw).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!( | ||
| "error parsing config proposal set facet json: {err}" | ||
| )) | ||
| })?; | ||
| return serde_json::from_value::<PseudoLabelCandidatesFacet>(facet_raw).map_err( | ||
| |err| { | ||
| JobErrorX::Terminal(ferr!("config facet is not pseudo label candidates: {err}")) | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| let value = PseudoLabelCandidatesFacet { labels: vec![] }; | ||
| let raw = token | ||
| .get() | ||
| .map_err(|err| JobErrorX::Terminal(ferr!("error reading config facet: {err:?}")))?; | ||
| let facet_raw: daybook_types::doc::FacetRaw = | ||
| serde_json::to_value(value.clone()).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!("error serializing default proposal set: {err}")) | ||
| serde_json::from_str(&raw).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!("error parsing config proposal set facet json: {err}")) | ||
| })?; | ||
| let facet_raw = serde_json::to_string(&facet_raw).expect(ERROR_JSON); | ||
| token | ||
| .update(&facet_raw) | ||
| .wrap_err("error writing default learned proposal set") | ||
| .map_err(JobErrorX::Terminal)?; | ||
| return Ok(value); | ||
| } | ||
|
|
||
| if let Some(token) = ro_config_token { | ||
| if !token.exists() { | ||
| return Ok(PseudoLabelCandidatesFacet { labels: vec![] }); | ||
| } | ||
| let raw = token.get(); | ||
| serde_json::from_value::<PseudoLabelCandidatesFacet>(facet_raw).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!("config facet is not pseudo label candidates: {err}")) | ||
| }) | ||
| } else if let Some(token) = ro_config_token { | ||
| let raw = token | ||
| .get() | ||
| .map_err(|err| JobErrorX::Terminal(ferr!("error reading ro config facet: {err:?}")))?; | ||
| let facet_raw: daybook_types::doc::FacetRaw = | ||
| serde_json::from_str(&raw).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!( | ||
| "error parsing ro config proposal set facet json: {err}" | ||
| )) | ||
| })?; | ||
| return serde_json::from_value::<PseudoLabelCandidatesFacet>(facet_raw).map_err(|err| { | ||
| serde_json::from_value::<PseudoLabelCandidatesFacet>(facet_raw).map_err(|err| { | ||
| JobErrorX::Terminal(ferr!( | ||
| "ro config facet is not pseudo label candidates: {err}" | ||
| )) | ||
| }); | ||
| }) | ||
| } else { | ||
| Ok(PseudoLabelCandidatesFacet { labels: vec![] }) | ||
| } |
There was a problem hiding this comment.
Load the proposal set from the readable token, not whichever token can update.
load_or_init_proposal_set tries rw_config_token first and calls get() on it. Under the new ACL split, a write-only config grant turns this into a hard failure instead of falling back to ro_config_token or the empty proposal set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plug_plabels/wflows/learn_image_label_candidates.rs` around lines 198 -
230, The function load_or_init_proposal_set currently tries rw_config_token
first and fails on get() for write-only grants; change the logic to prefer the
readable token: attempt to read from ro_config_token.get() if ro_config_token is
Some, parse that into PseudoLabelCandidatesFacet, and only if ro_config_token is
None then try rw_config_token.get() (so write-only tokens are only used as a
fallback). Update the branches that call get(), serde_json::from_str and
serde_json::from_value for the tokens (references: load_or_init_proposal_set,
rw_config_token, ro_config_token, get(), PseudoLabelCandidatesFacet) to reflect
this order and preserve the existing error mapping.
| "report-full-processor".into(), | ||
| Arc::new(CommandManifest { | ||
| desc: "report capabilities (full processor)".into(), | ||
| deets: CommandDeets::DocCommand { | ||
| routine_name: "report-full-processor".into(), | ||
| }, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
report-*-processor won't exercise the processor path as currently wired.
Both entries are still CommandDeets::DocCommand, so report_capabilities will always observe RoutineInvocation::Command for them. If these are meant to cover processor ACLs/invocation, they need real processors manifest entries rather than command aliases.
Also applies to: 1249-1256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plug_test/lib.rs` around lines 1231 - 1238, The two manifest entries like
the "report-full-processor" CommandManifest are using CommandDeets::DocCommand
so report_capabilities will only see RoutineInvocation::Command; replace those
DocCommand variants with actual processor manifest entries (e.g., use a
CommandDeets variant that lists processors or a Processor-specific manifest
type) so they register as processors rather than command aliases and thus
exercise the processor ACL/invocation path; make the same change for the sibling
report-* -processor entry as well and ensure the manifest field names match the
code that builds RoutineInvocation::Processor.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation