Conversation
- add unit, integration, and black-box CLI coverage for the settled 09-alice-bio-referenced-woven transition - extend local weave planning and runtime candidate loading to support first ReferenceCatalog weaving on an already-versioned Knop surface - version alice/_knop/_references into _history001/_s0001 and advance alice/_knop/_inventory to _s0002 while keeping _mesh/_inventory unchanged - extend the shared page-rendering seam to cover current and historical ReferenceCatalog pages, including the current #reference001 anchor - update the weave behavior note, codebase overview, decision log, and task note checklist for the carried 08 -> 09 slice
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ReferenceCatalog weaving (08→09) and a new payload.update flow (09→10): core planning, runtime execution with staging/rollback, CLI command wiring, page-rendering updates, validation and parsing, tests (unit/integration/e2e), docs, and small API re-exports. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client CLI
participant Runtime as Runtime Executor\n(executePayloadUpdate)
participant Core as Core Planner\n(planPayloadUpdate)
participant Validator as RDF Validator/Parser
participant FS as Filesystem (workspace)
participant Logger as Operational/Audit Log
CLI->>Runtime: invoke payload update (source, designatorPath, workspace)
activate Runtime
rect rgba(100,150,240,0.5)
Runtime->>Core: resolve inputs & planPayloadUpdate(resolved)
Core-->>Runtime: PayloadUpdatePlan
Runtime->>Validator: parse/validate updated TTL contents
Validator-->>Runtime: validated
Runtime->>FS: write staged temp file
FS-->>Runtime: temp written
Runtime->>FS: backup original
FS-->>Runtime: backup done
Runtime->>FS: rename staged into place
FS-->>Runtime: rename/replace success
Runtime->>Logger: emit operational & audit entries
end
deactivate Runtime
Runtime-->>CLI: PayloadUpdateResult (updatedPaths)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/weave/weave.ts (1)
230-250:⚠️ Potential issue | 🟠 MajorDon't dereference settled artifacts before you know the candidate is eligible.
loadPayloadWorkingArtifact()andloadReferenceCatalogWorkingArtifact()run beforeisWeaveableKnopCandidate(). That makesloadWeaveableKnopCandidates()fail on already-settled or non-requested knops if one of their old working files is missing, instead of skipping them.🩹 Sketch
- const candidate: WeaveableKnopCandidate = { + const payloadHasHistory = currentKnopInventoryTurtle.includes( + `sflo:hasArtifactHistory <${designatorPath}/_history001>`, + ); + const referenceCatalogPath = `${knopPath}/_references`; + const referenceCatalogHasHistory = currentKnopInventoryTurtle.includes( + `sflo:hasArtifactHistory <${referenceCatalogPath}/_history001>`, + ); + + const candidate: WeaveableKnopCandidate = { designatorPath, currentKnopMetadataTurtle, currentKnopInventoryTurtle, - payloadArtifact: await loadPayloadWorkingArtifact( - workspaceRoot, - designatorPath, - currentKnopInventoryTurtle, - ), - referenceCatalogArtifact: await loadReferenceCatalogWorkingArtifact( - workspaceRoot, - designatorPath, - currentKnopInventoryTurtle, - ), }; + if (!payloadHasHistory) { + candidate.payloadArtifact = await loadPayloadWorkingArtifact( + workspaceRoot, + designatorPath, + currentKnopInventoryTurtle, + ); + } + if (!referenceCatalogHasHistory) { + candidate.referenceCatalogArtifact = + await loadReferenceCatalogWorkingArtifact( + workspaceRoot, + designatorPath, + currentKnopInventoryTurtle, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/weave/weave.ts` around lines 230 - 250, The code eagerly calls loadPayloadWorkingArtifact and loadReferenceCatalogWorkingArtifact before checking eligibility, causing failures for already-settled or non-requested knops; update loadWeaveableKnopCandidates so you first build a minimal candidate containing designatorPath, currentKnopMetadataTurtle and currentKnopInventoryTurtle, call isWeaveableKnopCandidate(candidate) and only if it returns true then call loadPayloadWorkingArtifact(...) and loadReferenceCatalogWorkingArtifact(...) to attach payloadArtifact and referenceCatalogArtifact before pushing the candidate.
🧹 Nitpick comments (2)
src/runtime/weave/pages_test.ts (1)
36-74: Add a regression case for HTML-escaping in rendered link fieldsNice coverage for the first current-link case. I recommend adding one test with special characters in
fragment/referenceRoleLabel/referenceTargetPathso escaping behavior is locked in and regressions are caught early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/weave/pages_test.ts` around lines 36 - 74, Add a new Deno.test in pages_test.ts that calls renderResourcePage (same pattern as the existing "renderResourcePage renders current ReferenceCatalog pages with fragment anchors" test) but uses a currentLinks entry whose fragment, referenceRoleLabel, and referenceTargetPath contain special HTML characters (e.g., &, <, >, "). Assert the produced HTML contains the properly escaped entities (e.g., &, <, >, ") in the rendered list item and id attribute where appropriate to lock in escaping behavior for renderResourcePage.src/runtime/weave/weave.ts (1)
368-401: Share slice detection instead of duplicating it here.This predicate is now a second copy of the marker logic and precedence in
src/core/weave/weave.ts'sclassifyWeaveSlice(). If one side changes first, the runtime can silently filter a candidate that the planner would accept, or vice versa.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/weave/weave.ts` around lines 368 - 401, The predicate in isWeaveableKnopCandidate duplicates the slice/marker detection and precedence logic found in classifyWeaveSlice; extract the shared detection logic into a single exported utility (e.g., isWeaveableSlice or hasWeaveMarkers) that accepts the same inputs used here (candidate.currentKnopInventoryTurtle and candidate.designatorPath or equivalent), then replace the body of isWeaveableKnopCandidate and the logic inside classifyWeaveSlice to call that new shared function; ensure the new utility returns the same boolean semantics (check referenceCatalogRelationship, payloadRelationship, and knopInventoryHasHistory) and still uses referenceCatalogArtifact/payloadArtifact presence checks via the candidate inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/weave/weave.ts`:
- Around line 394-449: The planFirstReferenceCatalogWeave currently hardcodes
"/_references/.../references.ttl" instead of using
referenceCatalogArtifact.workingFilePath, so update
planFirstReferenceCatalogWeave to preserve
referenceCatalogArtifact.workingFilePath: compute
referenceCatalogWorkingFilePath from referenceCatalogArtifact.workingFilePath
and use it when building referenceCatalogPath and the createdFiles history
snapshot path and when calling
renderFirstReferenceCatalogWovenKnopInventoryTurtle (or extend that function to
accept the working file path so it can render hasWorkingLocatedFile and history
manifestation paths correctly). Also either adjust
assertCurrentKnopInventoryShapeForFirstReferenceCatalogWeave to reject
unsupported workingFilePath values or ensure the new code supports
non-"references.ttl" names so the inventory and created files point at the
actual working file path.
In `@src/runtime/weave/pages.ts`:
- Around line 51-53: Dynamic values interpolated into the HTML
(page.currentLinks -> currentLinks string, specifically link.fragment,
link.referenceRoleLabel, and link.referenceTargetPath) are not escaped, allowing
markup breakage or XSS; fix by HTML-escaping all interpolated values before
building the string (introduce or reuse an escapeHtml utility and apply it to
link.fragment for the id and display, and to link.referenceRoleLabel and
link.referenceTargetPath for the text/code content), and apply the same escaping
to the other similar block referenced at lines 64-66 so all dynamic HTML
fragments are safely encoded.
---
Outside diff comments:
In `@src/runtime/weave/weave.ts`:
- Around line 230-250: The code eagerly calls loadPayloadWorkingArtifact and
loadReferenceCatalogWorkingArtifact before checking eligibility, causing
failures for already-settled or non-requested knops; update
loadWeaveableKnopCandidates so you first build a minimal candidate containing
designatorPath, currentKnopMetadataTurtle and currentKnopInventoryTurtle, call
isWeaveableKnopCandidate(candidate) and only if it returns true then call
loadPayloadWorkingArtifact(...) and loadReferenceCatalogWorkingArtifact(...) to
attach payloadArtifact and referenceCatalogArtifact before pushing the
candidate.
---
Nitpick comments:
In `@src/runtime/weave/pages_test.ts`:
- Around line 36-74: Add a new Deno.test in pages_test.ts that calls
renderResourcePage (same pattern as the existing "renderResourcePage renders
current ReferenceCatalog pages with fragment anchors" test) but uses a
currentLinks entry whose fragment, referenceRoleLabel, and referenceTargetPath
contain special HTML characters (e.g., &, <, >, "). Assert the produced HTML
contains the properly escaped entities (e.g., &, <, >, ") in the
rendered list item and id attribute where appropriate to lock in escaping
behavior for renderResourcePage.
In `@src/runtime/weave/weave.ts`:
- Around line 368-401: The predicate in isWeaveableKnopCandidate duplicates the
slice/marker detection and precedence logic found in classifyWeaveSlice; extract
the shared detection logic into a single exported utility (e.g.,
isWeaveableSlice or hasWeaveMarkers) that accepts the same inputs used here
(candidate.currentKnopInventoryTurtle and candidate.designatorPath or
equivalent), then replace the body of isWeaveableKnopCandidate and the logic
inside classifyWeaveSlice to call that new shared function; ensure the new
utility returns the same boolean semantics (check referenceCatalogRelationship,
payloadRelationship, and knopInventoryHasHistory) and still uses
referenceCatalogArtifact/payloadArtifact presence checks via the candidate
inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8dcfc3b-6a20-49f2-a07e-e4d1cdb90f7f
📒 Files selected for processing (11)
documentation/notes/wd.codebase-overview.mddocumentation/notes/wd.decision-log.mddocumentation/notes/wd.spec.2026-04-03-weave-behavior.mddocumentation/notes/wd.task.2026.2026-04-04_1553-weave-alice-bio-referenced-woven.mdsrc/core/weave/weave.tssrc/core/weave/weave_test.tssrc/runtime/weave/pages.tssrc/runtime/weave/pages_test.tssrc/runtime/weave/weave.tstests/e2e/weave_cli_test.tstests/integration/weave_test.ts
- add the first narrow payload.update core/runtime path for an already woven payload artifact - expose the local CLI as `weave payload update <source> [designatorPath]` - replace the existing working payload file in place while leaving inventories, histories, and pages unchanged - allow runtime-local staging from a local path or file URL without polluting the shared semantic contract with host-path semantics - add unit, integration, and manifest-scoped black-box CLI coverage for 10-alice-bio-updated - add a dedicated payload.update behavior note and thin semantic-flow-framework request/job examples
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/cli/run.ts (1)
412-440: Consider deduplicating designator-path resolution logic.
resolvePayloadUpdateDesignatorPathnow mirrorsresolveIntegrateDesignatorPathalmost exactly. A shared helper would reduce behavior drift across commands over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/run.ts` around lines 412 - 440, Extract the duplicated logic in resolvePayloadUpdateDesignatorPath and resolveIntegrateDesignatorPath into a single helper (e.g., resolveDesignatorPath(options: { designatorPath?: string }, designatorPathArg?: string): string) and replace both implementations to call this helper; ensure the helper preserves current behavior and throws PayloadUpdateInputError (or a shared error type) with the same messages used by resolvePayloadUpdateDesignatorPath and resolveIntegrateDesignatorPath so callers see identical validation and error text, and update references to the original functions to use the new helper internally.src/core/payload/update.ts (2)
1-3: Consider extracting shared constants.
reservedDesignatorSegmentsis also defined insrc/runtime/payload/update.ts(line 12). While the duplication is acceptable to maintain proper core/runtime dependency boundaries, consider extracting to a shared constants module if this set grows or is used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/payload/update.ts` around lines 1 - 3, The duplicated constant reservedDesignatorSegments (used in update.ts and another runtime update.ts) should be extracted into a single exported constant in a shared module (e.g., create and export RESERVED_DESIGNATOR_SEGMENTS from a new/shared constants file) and both update.ts files should import that exported symbol instead of defining their own Set; update the references to use the shared RESERVED_DESIGNATOR_SEGMENTS (or the chosen exported name) so the set is maintained in one place while preserving existing dependency boundaries.
166-187: String-based RDF fragment matching is fragile but acceptable for narrow scope.Using
includes()for RDF shape validation works for the current "settled woven payload shape" where format is predictable, but could fail with equivalent RDF that has different whitespace, statement ordering, or prefixing. For future extensibility, consider parsing with an RDF library (e.g., N3) and checking triples semantically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/payload/update.ts` around lines 166 - 187, The current assertCurrentPayloadArtifactShape uses brittle string matching (currentKnopInventoryTurtle.includes on requiredFragments) which can fail for equivalent RDF with different whitespace/order/prefixes; replace the string-based checks in assertCurrentPayloadArtifactShape with a semantic RDF check by parsing currentKnopInventoryTurtle using an RDF parser (e.g., N3.Parser), convert triples/quads into a set you can query, and then verify the expected triples/properties (the knop resource a sflo:Knop, sflo:hasPayloadArtifact linking to the designatorPath, the designatorPath typed as sflo:PayloadArtifact/sflo:DigitalArtifact/sflo:RdfDocument, sflo:hasWorkingLocatedFile linking to workingFilePath, and sflo:currentArtifactHistory pointing at designatorPath/_history001) using triple membership tests against the parsed graph; keep the same PayloadUpdateInputError behavior when any required triple is missing and continue to reference the same function name and variables (currentKnopInventoryTurtle, designatorPath, workingFilePath, requiredFragments) to locate where to implement the parser-based checks.src/runtime/payload/update.ts (2)
433-450: Assumes exactly one file in the update plan.Line 437 accesses
plan.updatedFiles[0]!without checking length. While the currentplanPayloadUpdateimplementation always returns exactly one file, this assumption isn't enforced by the type system. Consider adding an assertion or handling multiple files.🛡️ Defensive assertion
async function applyPayloadUpdateAtomically( workspaceRoot: string, plan: PayloadUpdatePlan, ): Promise<void> { + if (plan.updatedFiles.length !== 1) { + throw new PayloadUpdateRuntimeError( + `Expected exactly one file to update, got ${plan.updatedFiles.length}`, + ); + } const file = plan.updatedFiles[0]!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/payload/update.ts` around lines 433 - 450, applyPayloadUpdateAtomically currently assumes exactly one file by using plan.updatedFiles[0] without checks; add a defensive check at the start of applyPayloadUpdateAtomically to validate plan.updatedFiles.length === 1 (or handle multiple files), and if the assertion fails throw a clear Error (or iterate and process each file consistently), so that the subsequent creation of stagedPayloadUpdate (absolutePath, tempPath, backupPath) is only done when a valid single-file invariant holds or is adapted to multi-file behavior.
396-413: Consider using asyncDeno.statfor consistency.This function uses synchronous
Deno.statSyncwhile the rest of the module uses async operations. While the impact is minimal for single-file operations, async would be more consistent with the codebase patterns.♻️ Suggested async version
-function assertUpdatedTargetsExist( +async function assertUpdatedTargetsExist( workspaceRoot: string, plan: PayloadUpdatePlan, -): void { +): Promise<void> { for (const file of plan.updatedFiles) { const absolutePath = join(workspaceRoot, file.path); try { - Deno.statSync(absolutePath); + await Deno.stat(absolutePath); } catch (error) { if (error instanceof Deno.errors.NotFound) {And update the call site at line 97:
- assertUpdatedTargetsExist(workspaceRoot, plan); + await assertUpdatedTargetsExist(workspaceRoot, plan);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/payload/update.ts` around lines 396 - 413, The function assertUpdatedTargetsExist currently uses synchronous Deno.statSync; change it to an async function (export or function signature: async function assertUpdatedTargetsExist(...): Promise<void>) and replace Deno.statSync(absolutePath) with await Deno.stat(absolutePath) inside a try/catch, preserving the same NotFound handling (if (error instanceof Deno.errors.NotFound) throw new PayloadUpdateRuntimeError(...); else rethrow). Finally update every call site (the caller that invoked assertUpdatedTargetsExist) to await the function so it returns a Promise and the control flow remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/payload/update_test.ts`:
- Line 32: The Deno.test declaration named "planPayloadUpdate rejects an
inventory that does not resolve the woven payload artifact" currently declares
its callback as async but contains no await (triggers require-await); remove the
unnecessary async modifier from the test callback so the function is a plain
synchronous function, keeping the test body unchanged and rerunning lint/tests
to confirm the require-await error is resolved.
In `@src/runtime/payload/update.ts`:
- Around line 170-174: The message returned by describePayloadUpdateResult
should remove the redundant "replacing" vs "updating" phrasing and correctly
pluralize "file"/"files" based on updatedPaths.length; update the function
(describePayloadUpdateResult) to build a single clear sentence using
result.payloadArtifactIri and result.workingFilePath, e.g. "Updated payload
<iri> by replacing working file <path> (updated N path(s))." but implement
proper plural logic using result.updatedPaths.length to select "file" vs "files"
and/or "path" vs "paths" so the grammar is correct.
---
Nitpick comments:
In `@src/cli/run.ts`:
- Around line 412-440: Extract the duplicated logic in
resolvePayloadUpdateDesignatorPath and resolveIntegrateDesignatorPath into a
single helper (e.g., resolveDesignatorPath(options: { designatorPath?: string },
designatorPathArg?: string): string) and replace both implementations to call
this helper; ensure the helper preserves current behavior and throws
PayloadUpdateInputError (or a shared error type) with the same messages used by
resolvePayloadUpdateDesignatorPath and resolveIntegrateDesignatorPath so callers
see identical validation and error text, and update references to the original
functions to use the new helper internally.
In `@src/core/payload/update.ts`:
- Around line 1-3: The duplicated constant reservedDesignatorSegments (used in
update.ts and another runtime update.ts) should be extracted into a single
exported constant in a shared module (e.g., create and export
RESERVED_DESIGNATOR_SEGMENTS from a new/shared constants file) and both
update.ts files should import that exported symbol instead of defining their own
Set; update the references to use the shared RESERVED_DESIGNATOR_SEGMENTS (or
the chosen exported name) so the set is maintained in one place while preserving
existing dependency boundaries.
- Around line 166-187: The current assertCurrentPayloadArtifactShape uses
brittle string matching (currentKnopInventoryTurtle.includes on
requiredFragments) which can fail for equivalent RDF with different
whitespace/order/prefixes; replace the string-based checks in
assertCurrentPayloadArtifactShape with a semantic RDF check by parsing
currentKnopInventoryTurtle using an RDF parser (e.g., N3.Parser), convert
triples/quads into a set you can query, and then verify the expected
triples/properties (the knop resource a sflo:Knop, sflo:hasPayloadArtifact
linking to the designatorPath, the designatorPath typed as
sflo:PayloadArtifact/sflo:DigitalArtifact/sflo:RdfDocument,
sflo:hasWorkingLocatedFile linking to workingFilePath, and
sflo:currentArtifactHistory pointing at designatorPath/_history001) using triple
membership tests against the parsed graph; keep the same PayloadUpdateInputError
behavior when any required triple is missing and continue to reference the same
function name and variables (currentKnopInventoryTurtle, designatorPath,
workingFilePath, requiredFragments) to locate where to implement the
parser-based checks.
In `@src/runtime/payload/update.ts`:
- Around line 433-450: applyPayloadUpdateAtomically currently assumes exactly
one file by using plan.updatedFiles[0] without checks; add a defensive check at
the start of applyPayloadUpdateAtomically to validate plan.updatedFiles.length
=== 1 (or handle multiple files), and if the assertion fails throw a clear Error
(or iterate and process each file consistently), so that the subsequent creation
of stagedPayloadUpdate (absolutePath, tempPath, backupPath) is only done when a
valid single-file invariant holds or is adapted to multi-file behavior.
- Around line 396-413: The function assertUpdatedTargetsExist currently uses
synchronous Deno.statSync; change it to an async function (export or function
signature: async function assertUpdatedTargetsExist(...): Promise<void>) and
replace Deno.statSync(absolutePath) with await Deno.stat(absolutePath) inside a
try/catch, preserving the same NotFound handling (if (error instanceof
Deno.errors.NotFound) throw new PayloadUpdateRuntimeError(...); else rethrow).
Finally update every call site (the caller that invoked
assertUpdatedTargetsExist) to await the function so it returns a Promise and the
control flow remains correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bab4d17f-0eb4-4e11-814d-9b3add4ca3ae
📒 Files selected for processing (14)
documentation/notes/wd.codebase-overview.mddocumentation/notes/wd.decision-log.mddocumentation/notes/wd.spec.2026-04-04-payload-update-behavior.mddocumentation/notes/wd.task.2026.2026-04-04_2019-update-alice-bio-payload.mdsrc/cli/run.tssrc/core/mod.tssrc/core/payload/mod.tssrc/core/payload/update.tssrc/core/payload/update_test.tssrc/runtime/mod.tssrc/runtime/payload/mod.tssrc/runtime/payload/update.tstests/e2e/payload_update_cli_test.tstests/integration/payload_update_test.ts
✅ Files skipped from review due to trivial changes (4)
- src/core/mod.ts
- src/runtime/mod.ts
- documentation/notes/wd.spec.2026-04-04-payload-update-behavior.md
- documentation/notes/wd.decision-log.md
Comment on lines
+170
to
+174
| export function describePayloadUpdateResult( | ||
| result: PayloadUpdateResult, | ||
| ): string { | ||
| return `Updated payload ${result.payloadArtifactIri} by replacing ${result.workingFilePath} and updating ${result.updatedPaths.length} working file.`; | ||
| } |
There was a problem hiding this comment.
Clarify result description message.
The message has minor issues:
- Uses singular "file" regardless of count (though currently always 1)
- Mentions both "replacing" and "updating" which describe the same operation
📝 Suggested fix
export function describePayloadUpdateResult(
result: PayloadUpdateResult,
): string {
- return `Updated payload ${result.payloadArtifactIri} by replacing ${result.workingFilePath} and updating ${result.updatedPaths.length} working file.`;
+ return `Updated payload ${result.payloadArtifactIri} by replacing working file ${result.workingFilePath}.`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/payload/update.ts` around lines 170 - 174, The message returned
by describePayloadUpdateResult should remove the redundant "replacing" vs
"updating" phrasing and correctly pluralize "file"/"files" based on
updatedPaths.length; update the function (describePayloadUpdateResult) to build
a single clear sentence using result.payloadArtifactIri and
result.workingFilePath, e.g. "Updated payload <iri> by replacing working file
<path> (updated N path(s))." but implement proper plural logic using
result.updatedPaths.length to select "file" vs "files" and/or "path" vs "paths"
so the grammar is correct.
… and HTML escaping
- improve payload update result wording with correct singular/plural grammar - share CLI designator-path resolution between integrate and payload update - move payload reserved designator segments into a shared core constant - replace payload.update woven-shape string matching with parser-backed quad checks - make payload target existence checks async and guard the single-file atomic update invariant - add unit coverage for semantic-equivalent payload inventory parsing and result message grammar
- file: URL payload sources, which the runtime explicitly supports in update.ts - invalid Turtle replacement input, with an assertion that the working payload file is left unchanged - CLI rejection for conflicting designator paths - CLI rejection when the designator path is missing, and confirmation that logging/execution never starts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/runtime/weave/weave.ts (1)
238-274: Minor inefficiency:detectPendingWeaveSlicecalled twice per candidate.The slice is detected at Line 243-246 and again inside
isWeaveableKnopCandidateat Line 392-395. Consider passing the already-computed slice to avoid redundant string parsing.♻️ Suggested refactor
- if (!isWeaveableKnopCandidate(candidate)) { + if (!isWeaveableKnopCandidate(candidate, slice)) { continue; }And update the function signature:
-function isWeaveableKnopCandidate(candidate: WeaveableKnopCandidate): boolean { - const slice = detectPendingWeaveSlice( - candidate.designatorPath, - candidate.currentKnopInventoryTurtle, - ); +function isWeaveableKnopCandidate( + candidate: WeaveableKnopCandidate, + slice: WeaveSlice | undefined, +): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/weave/weave.ts` around lines 238 - 274, You are calling detectPendingWeaveSlice twice for each candidate; compute the slice once (as you already do into variable slice) and thread it through instead of re-parsing inside isWeaveableKnopCandidate. Update isWeaveableKnopCandidate to accept the precomputed slice (e.g., add a parameter like candidateSlice) or create a new helper (e.g., isWeaveableKnopCandidateWithSlice) and adjust callers here so you use the existing slice to decide whether to load payload/reference via loadPayloadWorkingArtifact/loadReferenceCatalogWorkingArtifact and to validate the candidate before pushing to candidates.src/core/weave/weave.ts (1)
137-141: Unreachable code after exhaustive switch.The switch at Lines 117-136 handles all three
WeaveSlicecases (firstKnopWeave,firstPayloadWeave,firstReferenceCatalogWeave). IfclassifyWeaveSlicereturnsundefinedfor unsupported candidates, this code would be reached. However, TypeScript's control flow analysis may not detect this as exhaustive due to theundefinedreturn type.Consider either:
- Having
classifyWeaveSlicethrow for unsupported slices instead of returningundefined- Adding an explicit
defaultcase in the switchThis is currently safe because
classifyWeaveSlicethrows for missing artifacts, but the logic could be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/weave/weave.ts` around lines 137 - 141, The switch over classifyWeaveSlice results (handling firstKnopWeave, firstPayloadWeave, firstReferenceCatalogWeave) is treated as exhaustive but TypeScript may still consider an undefined return path; make the control flow explicit by either changing classifyWeaveSlice to throw on unsupported slices or adding a default branch to the switch that throws the WeaveInputError; locate the switch that consumes classifyWeaveSlice and either update classifyWeaveSlice to throw for unsupported candidates or add an explicit default case that throws the existing WeaveInputError (the throw new WeaveInputError(...) statement) so there is no unreachable/implicit code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 26: Replace the malformed guidance sentence "After any round of
significan code changes, run lint and provide a reasonably-detailed commit
message," with a corrected, polished version: fix the double space and spelling
("round", "significant"), remove the trailing comma, and prefer "run the linter"
and "reasonably detailed commit message" (no hyphen). For example: "After any
round of significant code changes, run the linter and provide a reasonably
detailed commit message."
In `@src/core/weave/weave.ts`:
- Around line 517-536: The function
assertCurrentMeshInventoryShapeForFirstReferenceCatalogWeave currently hardcodes
two designator paths ("<alice/_knop>" and "<alice/bio/_knop>") which couples the
validation to the 08-alice-bio fixture; update this function to either (A)
accept the expected knop designators as parameters (e.g., pass an array of
required designator prefixes into
assertCurrentMeshInventoryShapeForFirstReferenceCatalogWeave) and check for
those parameterized fragments instead of literal "<alice/_knop>" and
"<alice/bio/_knop>", or (B) remove the two specific knop fragments from
requiredFragments and replace them with a more general check (e.g., assert
presence of any "a sflo:Knop" triple or a regex like /<[^>]+> a sflo:Knop/
against currentMeshInventoryTurtle) so the precondition is not tied to the alice
fixture; update callers accordingly to provide expected designators when needed.
In `@src/runtime/payload/update.ts`:
- Around line 292-311: resolveSourcePath currently sends every source string
through tryParseUrl which misclassifies Windows paths and colon-containing
filenames as URLs; change the logic to only treat inputs that explicitly start
with "file:" as URLs (parse them with fromFileUrl/new URL), detect and reject
obvious remote schemes of the form "<scheme>://" by throwing the same
PayloadUpdateRuntimeError for non-file protocols, and otherwise treat the input
as a filesystem path (using isAbsolute/resolve with workspaceRoot). Update or
remove tryParseUrl usage accordingly so resolveSourcePath no longer parses
generic strings through URL constructor.
---
Nitpick comments:
In `@src/core/weave/weave.ts`:
- Around line 137-141: The switch over classifyWeaveSlice results (handling
firstKnopWeave, firstPayloadWeave, firstReferenceCatalogWeave) is treated as
exhaustive but TypeScript may still consider an undefined return path; make the
control flow explicit by either changing classifyWeaveSlice to throw on
unsupported slices or adding a default branch to the switch that throws the
WeaveInputError; locate the switch that consumes classifyWeaveSlice and either
update classifyWeaveSlice to throw for unsupported candidates or add an explicit
default case that throws the existing WeaveInputError (the throw new
WeaveInputError(...) statement) so there is no unreachable/implicit code path.
In `@src/runtime/weave/weave.ts`:
- Around line 238-274: You are calling detectPendingWeaveSlice twice for each
candidate; compute the slice once (as you already do into variable slice) and
thread it through instead of re-parsing inside isWeaveableKnopCandidate. Update
isWeaveableKnopCandidate to accept the precomputed slice (e.g., add a parameter
like candidateSlice) or create a new helper (e.g.,
isWeaveableKnopCandidateWithSlice) and adjust callers here so you use the
existing slice to decide whether to load payload/reference via
loadPayloadWorkingArtifact/loadReferenceCatalogWorkingArtifact and to validate
the candidate before pushing to candidates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd182702-1e85-4958-aaa5-3e8c127135f8
📒 Files selected for processing (13)
AGENTS.mdsrc/cli/run.tssrc/core/designator_segments.tssrc/core/payload/update.tssrc/core/payload/update_test.tssrc/core/weave/weave.tssrc/core/weave/weave_test.tssrc/runtime/payload/update.tssrc/runtime/payload/update_test.tssrc/runtime/weave/pages.tssrc/runtime/weave/pages_test.tssrc/runtime/weave/weave.tstests/integration/weave_test.ts
✅ Files skipped from review due to trivial changes (2)
- src/core/designator_segments.ts
- src/core/payload/update.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/payload/update_test.ts
- src/runtime/weave/pages_test.ts
- src/core/weave/weave_test.ts
Comment on lines
+344
to
+350
| const meshBaseMatch = meshMetadataTurtle.match( | ||
| /sflo:meshBase "([^"]+)"\^\^xsd:anyURI/, | ||
| ); | ||
| if (!meshBaseMatch) { | ||
| throw new PayloadUpdateRuntimeError( | ||
| "Could not resolve meshBase from _mesh/_meta/meta.ttl", | ||
| ); |
There was a problem hiding this comment.
Parse the workspace Turtle semantically instead of matching serializer output.
These regex/string scans assume one exact Turtle layout. CRLF line endings, different blank-line grouping, reordered predicates/types, or absolute subject IRIs will make payload update fail even when the RDF is valid. They also join the extracted workingFilePath into the filesystem before that path has been normalized. Since this file already depends on n3, use quads here too and validate the path before the Deno.stat() call.
Also applies to: 353-377
…ading - remove alice-specific mesh-inventory assumptions from the first reference-catalog weave precondition - reuse the detected weave slice during runtime candidate loading instead of recomputing it - treat only explicit file: inputs as URL sources for payload update and reject remote schemes - keep colon-containing payload source filenames on the filesystem-path path - add focused weave and payload coverage for non-alice reference-catalog weaving and payload source resolution edge cases
- conflicting positional vs --designator-path -missing designator path entirely
This was referenced Apr 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests