feat(knop): implement first local add-reference slice#3
Merged
Conversation
- add core planning and local runtime execution for `knop.addReference` - wire `weave knop add-reference` into the CLI with explicit target designator and `referenceRole` - add unit, integration, and black-box CLI tests for the settled `07-alice-bio-integrated-woven` -> `08-alice-bio-referenced` fixture - add `[[wd.spec.2026-04-04-knop-add-reference-behavior]]` for the first carried reference-catalog behavior - rename the task note to completed and update `wd.todo`, `wd.decision-log`, `wd.codebase-overview`, and related conversation links
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a carried implementation slice for Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Handler
participant Runtime as Runtime Executor
participant Core as Core Planner
participant FS as Filesystem
participant Meta as Mesh Meta
CLI->>Runtime: executeKnopAddReference(workspaceRoot, request)
Runtime->>FS: validate workspace root & paths
Runtime->>Meta: read _mesh/_meta/meta.ttl
Meta-->>Runtime: meshBase
Runtime->>FS: read source knop inventory.ttl
FS-->>Runtime: currentKnopInventoryTurtle
Runtime->>Core: planKnopAddReference(resolved request)
Core->>Core: normalize inputs, derive IRIs & paths, render references.ttl and updated inventory.ttl
Core-->>Runtime: KnopAddReferencePlan
Runtime->>FS: verify target inventory exists & created files absent
Runtime->>FS: write created files (temp → atomic rename)
Runtime->>FS: stage backups and replace updated files
FS-->>Runtime: write success
Runtime-->>CLI: KnopAddReferenceResult (created/updated paths, IRIs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 docstrings
🧪 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: 4
🤖 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/cli/run.ts`:
- Around line 198-227: Trim and validate the positional designatorPath inside
the .action handler before using it: call designatorPath =
String(designatorPath).trim() (or equivalent) after it is received, then check
it is non-empty and if invalid throw/return a clear error (same pattern as
resolveRequiredOptionValue) so execution (including executeKnopAddReference and
auditLogger.command) never proceeds with a whitespace-only value; update any
error message to mention the positional designatorPath.
In `@src/core/knop/add_reference.ts`:
- Around line 50-68: normalizeDesignatorPath currently allows segments like
"alice:bio" that can produce non-mesh-relative IRIs; update
normalizeDesignatorPath to validate each segment against a safe segment charset
(only allow a-z, A-Z, 0-9, hyphen, underscore, dot — reject colons, slashes,
spaces, etc.), and throw a clear validation error when a segment fails. In
add_reference.ts (places constructing designatorPath and
referenceTargetDesignatorPath and where toKnopPath is used) call the tightened
normalizeDesignatorPath and ensure any invalid input causes fast failure rather
than producing IRIs; mirror the identical charset check in the runtime validator
code paths mentioned (the blocks around lines 117-153 and 168-185) so both
compile-time and runtime enforce the same rule. Ensure error messages reference
the offending segment and the function names normalizeDesignatorPath and
toKnopPath for easy debugging.
- Around line 155-165: The normalizeReferenceRole function currently uses the
"in" operator against referenceRoleIriByToken which matches prototype properties
(e.g., "constructor"); change the validation to check only own properties by
using Object.prototype.hasOwnProperty.call(referenceRoleIriByToken, normalized)
instead of "normalized in referenceRoleIriByToken", keep throwing
KnopAddReferenceInputError for empty/unsupported values and return the
normalized value as ReferenceRoleToken as before.
In `@src/runtime/knop/add_reference.ts`:
- Around line 102-108: The workspace mutation is not atomic because
writeCreatedFiles and writeUpdatedFiles commit changes before all work (and
success logging) completes; modify the add-reference flow so mutations are done
atomically: for created files use a staging-and-rename strategy (write to temp
paths then atomically rename into final paths) or implement a rollback path that
removes any newly created files if a later step fails, and ensure
writeUpdatedFiles applies updates transactionally (e.g., write to temp files and
replace). Keep the pre-checks (assertReferenceTargetExists,
assertCreateTargetsDoNotExist) as-is but perform the staged commit immediately
after those checks and before any post-commit logging; make the success log
calls best-effort (catch/log but don’t throw) so logging failures do not leave
the workspace mutated without allowing retries. Ensure the new behavior is
applied to both the block around writeCreatedFiles/writeUpdatedFiles and the
other occurrence noted (lines ~159-188) so all workspace mutations are atomic.
🪄 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: 9546b48c-2afa-467f-87ee-0df46aac2a4f
⛔ Files ignored due to path filters (3)
documentation/notes/wd.completed.2026.2026-04-04-alice-bio-referenced.mdis excluded by!documentation/notes/wd.completed.*documentation/notes/wd.conv.2026.2026-04-04_1524-weave-alice-bio-integrated-woven-codex.mdis excluded by!documentation/notes/wd.conv.*documentation/notes/wd.conv.2026.2026-04-04_1537-08-weave-alice-bio-referenced-codex.mdis excluded by!documentation/notes/wd.conv.*
📒 Files selected for processing (12)
documentation/notes/wd.codebase-overview.mddocumentation/notes/wd.decision-log.mddocumentation/notes/wd.spec.2026-04-04-knop-add-reference-behavior.mddocumentation/notes/wd.todo.mdsrc/cli/run.tssrc/core/knop/add_reference.tssrc/core/knop/add_reference_test.tssrc/core/knop/mod.tssrc/runtime/knop/add_reference.tssrc/runtime/knop/mod.tstests/e2e/knop_add_reference_cli_test.tstests/integration/knop_add_reference_test.ts
💤 Files with no reviewable changes (1)
- documentation/notes/wd.todo.md
Comment on lines
+50
to
+68
| const meshBase = normalizeMeshBase(request.meshBase); | ||
| const designatorPath = normalizeDesignatorPath(request.designatorPath); | ||
| const referenceTargetDesignatorPath = normalizeDesignatorPath( | ||
| request.referenceTargetDesignatorPath, | ||
| ); | ||
| const referenceRoleToken = normalizeReferenceRole(request.referenceRole); | ||
| const referenceRoleIri = referenceRoleIriByToken[referenceRoleToken]; | ||
| const knopPath = toKnopPath(designatorPath); | ||
| const referenceCatalogPath = `${knopPath}/_references`; | ||
| const referenceLinkPath = `${referenceCatalogPath}#reference001`; | ||
|
|
||
| return { | ||
| meshBase, | ||
| designatorPath, | ||
| referenceTargetDesignatorPath, | ||
| referenceCatalogIri: new URL(referenceCatalogPath, meshBase).href, | ||
| referenceLinkIri: new URL(referenceLinkPath, meshBase).href, | ||
| referenceRoleIri, | ||
| referenceTargetIri: new URL(referenceTargetDesignatorPath, meshBase).href, |
There was a problem hiding this comment.
Reject designator characters that stop being mesh-relative in RDF output.
normalizeDesignatorPath() still accepts values like alice:bio. Once those strings are turned into planned IRIs and raw Turtle <...> refs, the add-reference result can escape the mesh namespace or serialize invalid RDF instead of failing fast. Please lock this down to a safe segment charset here, and mirror the same rule in the runtime validator.
Also applies to: 117-153, 168-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/knop/add_reference.ts` around lines 50 - 68, normalizeDesignatorPath
currently allows segments like "alice:bio" that can produce non-mesh-relative
IRIs; update normalizeDesignatorPath to validate each segment against a safe
segment charset (only allow a-z, A-Z, 0-9, hyphen, underscore, dot — reject
colons, slashes, spaces, etc.), and throw a clear validation error when a
segment fails. In add_reference.ts (places constructing designatorPath and
referenceTargetDesignatorPath and where toKnopPath is used) call the tightened
normalizeDesignatorPath and ensure any invalid input causes fast failure rather
than producing IRIs; mirror the identical charset check in the runtime validator
code paths mentioned (the blocks around lines 117-153 and 168-185) so both
compile-time and runtime enforce the same rule. Ensure error messages reference
the offending segment and the function names normalizeDesignatorPath and
toKnopPath for easy debugging.
- validate and trim the positional `designatorPath` in the CLI - reject unsafe designator path segments in core and runtime normalization - use an own-property check for `referenceRole` token validation - stage add-reference workspace mutations atomically with rollback on failure - treat success logging as best-effort after commit - add regression tests for invalid inputs and logging-failure behavior
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/runtime/knop/add_reference.ts (1)
501-504:⚠️ Potential issue | 🟠 MajorThe updated-file swap still isn't atomic.
Line 502 moves the live file aside before Line 503 publishes the replacement. If the process is interrupted between those awaits, or another reader hits the workspace mid-commit, the updated path disappears even though this flow is labeled atomic. Please switch updated-file commits to a replace-in-place step so the destination path never goes missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/knop/add_reference.ts` around lines 501 - 504, The two-step swap in the loop (Deno.rename(file.absolutePath, file.backupPath!) followed by Deno.rename(file.tempPath, file.absolutePath)) is non-atomic because the destination can be missing between awaits; change this to an atomic replace by renaming the new file directly over the destination (use Deno.rename(file.tempPath, file.absolutePath)) so the target path is never missing. If you still need a backup, create it first with a non-destructive copy (e.g., Deno.copyFile(file.absolutePath, file.backupPath!)) and then atomically replace with Deno.rename(file.tempPath, file.absolutePath); update the loop in add_reference.ts to use these operations on the file.tempPath/file.absolutePath/file.backupPath symbols.
🤖 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/runtime/knop/add_reference.ts`:
- Around line 121-148: The catch block currently awaits operationalLogger.error
and auditLogger.record and can throw, masking the original
KnopAddReference*Error; change it to preserve the original error (capture it as
originalErr), compute the message, then call operationalLogger.error and
auditLogger.record inside their own try/catch blocks (best-effort: swallow/log
any logging failures but do not let them throw), and finally rethrow the
originalErr so the original validation/commit failure is propagated; reference
operationalLogger.error, auditLogger.record and the surrounding catch for the
addReference flow.
In `@tests/integration/knop_add_reference_test.ts`:
- Around line 151-169: The test sinks' async write(record) stubs in the
StructuredLogger used for throwingOperationalLogger and the inner
StructuredLogger passed to AuditLogger must explicitly return a Promise to
satisfy require-await; modify both write implementations (the write methods on
the StructuredLogger instances) to return Promise.reject(new Error("operational
success log failed")) / Promise.reject(new Error("audit success log failed"))
when record.event === "knop.addReference.succeeded" and return Promise.resolve()
otherwise so the failure injection still works but lint passes.
---
Duplicate comments:
In `@src/runtime/knop/add_reference.ts`:
- Around line 501-504: The two-step swap in the loop
(Deno.rename(file.absolutePath, file.backupPath!) followed by
Deno.rename(file.tempPath, file.absolutePath)) is non-atomic because the
destination can be missing between awaits; change this to an atomic replace by
renaming the new file directly over the destination (use
Deno.rename(file.tempPath, file.absolutePath)) so the target path is never
missing. If you still need a backup, create it first with a non-destructive copy
(e.g., Deno.copyFile(file.absolutePath, file.backupPath!)) and then atomically
replace with Deno.rename(file.tempPath, file.absolutePath); update the loop in
add_reference.ts to use these operations on the
file.tempPath/file.absolutePath/file.backupPath symbols.
🪄 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: d23acc5d-f9ff-4d33-9632-ef8eb7f3ce40
⛔ Files ignored due to path filters (1)
documentation/notes/wd.conv.2026.2026-04-04_1537-08-weave-alice-bio-referenced-codex.mdis excluded by!documentation/notes/wd.conv.*
📒 Files selected for processing (10)
AGENTS.mddocumentation/notes/wd.general-guidance.mddocumentation/notes/wd.task.2026.2026-04-04_1553-weave-alice-bio-referenced-woven.mddocumentation/notes/wd.todo.mdsrc/cli/run.tssrc/core/knop/add_reference.tssrc/core/knop/add_reference_test.tssrc/runtime/knop/add_reference.tstests/e2e/knop_add_reference_cli_test.tstests/integration/knop_add_reference_test.ts
✅ Files skipped from review due to trivial changes (4)
- AGENTS.md
- documentation/notes/wd.general-guidance.md
- src/core/knop/add_reference_test.ts
- src/core/knop/add_reference.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- documentation/notes/wd.todo.md
Comment on lines
+151
to
+169
| const throwingOperationalLogger = new StructuredLogger([{ | ||
| async write(record: { event: string }) { | ||
| if (record.event === "knop.addReference.succeeded") { | ||
| throw new Error("operational success log failed"); | ||
| } | ||
| }, | ||
| }], { | ||
| channel: "operational", | ||
| }); | ||
| const throwingAuditLogger = new AuditLogger( | ||
| new StructuredLogger([{ | ||
| async write(record: { event: string }) { | ||
| if (record.event === "knop.addReference.succeeded") { | ||
| throw new Error("audit success log failed"); | ||
| } | ||
| }, | ||
| }], { | ||
| channel: "security-audit", | ||
| }), |
There was a problem hiding this comment.
Fix the require-await CI failure in these test sinks.
The write() stubs on Line 152 and Line 162 never await, so deno lint is already failing. Return Promise.reject() / Promise.resolve() explicitly here so the failure injection still works without breaking CI.
Lint-safe rewrite
const throwingOperationalLogger = new StructuredLogger([{
- async write(record: { event: string }) {
+ write(record: { event: string }) {
if (record.event === "knop.addReference.succeeded") {
- throw new Error("operational success log failed");
+ return Promise.reject(new Error("operational success log failed"));
}
+ return Promise.resolve();
},
}], {
channel: "operational",
});
const throwingAuditLogger = new AuditLogger(
new StructuredLogger([{
- async write(record: { event: string }) {
+ write(record: { event: string }) {
if (record.event === "knop.addReference.succeeded") {
- throw new Error("audit success log failed");
+ return Promise.reject(new Error("audit success log failed"));
}
+ return Promise.resolve();
},
}], {📝 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.
Suggested change
| const throwingOperationalLogger = new StructuredLogger([{ | |
| async write(record: { event: string }) { | |
| if (record.event === "knop.addReference.succeeded") { | |
| throw new Error("operational success log failed"); | |
| } | |
| }, | |
| }], { | |
| channel: "operational", | |
| }); | |
| const throwingAuditLogger = new AuditLogger( | |
| new StructuredLogger([{ | |
| async write(record: { event: string }) { | |
| if (record.event === "knop.addReference.succeeded") { | |
| throw new Error("audit success log failed"); | |
| } | |
| }, | |
| }], { | |
| channel: "security-audit", | |
| }), | |
| const throwingOperationalLogger = new StructuredLogger([{ | |
| write(record: { event: string }) { | |
| if (record.event === "knop.addReference.succeeded") { | |
| return Promise.reject(new Error("operational success log failed")); | |
| } | |
| return Promise.resolve(); | |
| }, | |
| }], { | |
| channel: "operational", | |
| }); | |
| const throwingAuditLogger = new AuditLogger( | |
| new StructuredLogger([{ | |
| write(record: { event: string }) { | |
| if (record.event === "knop.addReference.succeeded") { | |
| return Promise.reject(new Error("audit success log failed")); | |
| } | |
| return Promise.resolve(); | |
| }, | |
| }], { | |
| channel: "security-audit", | |
| }), |
🧰 Tools
🪛 GitHub Actions: ci
[error] 152-152: deno lint reported error[require-await]: Async method 'write' has no 'await' expression or 'await using' declaration.
🪛 GitHub Check: ci
[failure] 162-162:
�[0m�[1mAsync method 'write' has no 'await' expression or 'await using' declaration.
[failure] 152-152:
�[0m�[1mAsync method 'write' has no 'await' expression or 'await using' declaration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/knop_add_reference_test.ts` around lines 151 - 169, The
test sinks' async write(record) stubs in the StructuredLogger used for
throwingOperationalLogger and the inner StructuredLogger passed to AuditLogger
must explicitly return a Promise to satisfy require-await; modify both write
implementations (the write methods on the StructuredLogger instances) to return
Promise.reject(new Error("operational success log failed")) / Promise.reject(new
Error("audit success log failed")) when record.event ===
"knop.addReference.succeeded" and return Promise.resolve() otherwise so the
failure injection still works but lint passes.
- preserve the original add-reference error when failed-path logging throws - use backup copy plus direct staged rename for updated-file commits - add regression coverage for failed-path logging
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.
knop.addReferenceweave knop add-referenceinto the CLI with explicit target designator andreferenceRole07-alice-bio-integrated-woven->08-alice-bio-referencedfixture[[wd.spec.2026-04-04-knop-add-reference-behavior]]for the first carried reference-catalog behaviorwd.todo,wd.decision-log,wd.codebase-overview, and related conversation links@CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests