Review/weave since v0.1.1 - #15
Closed
djradon wants to merge 11 commits into
Closed
Conversation
- replace cross-repo ontology guardrails with Weave defaults/runtime checks - remove the semantic-flow/sflo checkout from ordinary CI - preserve canonical namespace and retired-config fragment coverage for Weave-owned files
- document disposable v0.1.1 publication regeneration from /tmp/sflo - use working-only external source bindings without repository evidence - add validation checks for floating source registries and generated release paths
- collect RDF type hints for displayed child resources on pages being written - keep non-target sibling source lookup best-effort - cover SHACL NodeShape, PropertyShape, and Shape child rows plus the Fantasy Rules regression
- preserve RepositorySourceFloatingLocator payload bindings through payload weave output - accept floating repository source payloads during extraction and extracted-term weave validation - resolve floating repository source paths from operational source grants - normalize SSH/scp GitHub remotes when matching repository source URLs - update SFLO dogfood CLI example to use targeted weave and post-weave validation - add unit, integration, and e2e coverage for floating repository source bindings
- add command-scoped runtime timing support behind WEAVE_TIMING=1 - emit aggregate phase timings for validate, version, generate, and weave - aggregate repeated recursive planning phases with count and average duration - keep timing output on stderr so normal CLI output remains stable - add e2e coverage for timing output on weave commands
- add command-scoped cached file reads for weave planning overlays - cache weaveable candidate resolution across recursive planning passes - invalidate cached candidates when staged files touch recorded read dependencies - expose cache counters in WEAVE_TIMING output
…repository options, and integration; add tests for latest payload state handling
- resolve current RDF ResourcePage raw panels from latest historical manifestations for payload, Knop support, and mesh support artifacts when histories exist - choose preferred manifestations by working-file extension with deterministic fallback - allow inventory resolution for multi-manifestation states when locatedFileForState disambiguates the preferred file - preserve current-file panels for current-only support artifacts - add regression coverage for stale payload/support working files and multi-manifestation selection - track deferred ResourcePage findings and working-file metadata hiding in wd.todo
Add ResourcePage hero metadata rows for non-local working source locators. Working URLs now render as links, while repository-floating sources render as plain repo-root paths without pretending to be mesh-local files. Thread workingAccessUrl and repository floating locator data through inventory, generation context, and page models. Add unit and integration coverage for URL and repository-floating ResourcePage metadata.
Add ResourcePage hero metadata rows for non-local working source locators. Working URLs now render as links, while repository-floating sources render as plain repo-root paths without pretending to be mesh-local files. Thread workingAccessUrl and repository floating locator data through inventory, generation context, and page models. Add unit and integration coverage for URL and repository-floating ResourcePage metadata.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/weave/weave.ts (2)
1694-1750:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFloating-locator support stops before current source-panel loading.
This plumbs
repositorySourceFloatingLocatorintoPayloadWorkingArtifactand page context metadata, but the current-source panel paths for canonical references and extracted sources still open files throughresolveAllowedLocalPath(...)only. With a floating source payload,artifactResolutionMode_working(or a canonical reference without an explicit state) will silently drop its raw-source panel instead of resolving throughresolveRepositorySourceFloatingLocalPath(...).Also applies to: 2857-2865
🤖 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/runtime/weave/weave.ts` around lines 1694 - 1750, The current-source loading path stops honoring payloadArtifact.repositorySourceFloatingLocator because code always calls resolveAllowedLocalPath for the canonical/raw source files; update the branches that compute absolute paths for current-source/canonical references to first check payloadArtifact.repositorySourceFloatingLocator and, when present, call resolveRepositorySourceFloatingLocalPath(localPathPolicy, payloadArtifact.repositorySourceFloatingLocator) instead of resolveAllowedLocalPath; ensure the same logic is applied wherever resolveAllowedLocalPath is used for working/canonical source resolution (e.g., the block around readTextFileWithOverlay that sets currentPayloadTurtle and the similar block at lines ~2857-2865), and keep throwing the same LocalPathAccessError/Deno.errors.NotFound handling and preserve artifactResolutionMode_working / PayloadWorkingArtifact metadata plumbing.
680-699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the
tryboundary above the startup work.
loadOperationalLocalPathPolicy()and the initialweave.startedlogs run before thetry, so failures there skipweave.failedlogging, skiptiming.finish(), and can leak raw errors instead of the normal runtime wrapping.Suggested fix
export async function executeWeave( options: ExecuteWeaveOptions, ): Promise<WeaveResult> { const timing = createRuntimeTiming("weave"); let status = "succeeded"; const { operationalLogger, auditLogger } = resolveLoggers(options); const meshRoot = resolveExecutionMeshRoot(options); - const initialPolicy = await timing.time( - "loadOperationalLocalPathPolicy", - () => loadOperationalLocalPathPolicy(meshRoot), - ); - const workspaceRoot = initialPolicy.workspaceRoot; + let workspaceRoot = meshRoot; let wovenDesignatorPaths: readonly string[] = []; - - await operationalLogger.info("weave.started", "Starting local weave", { - workspaceRoot, - targets: options.request?.targets ?? [], - }); - await auditLogger.record("weave.started", "Local weave started", { - workspaceRoot, - targets: options.request?.targets ?? [], - }); try { + const initialPolicy = await timing.time( + "loadOperationalLocalPathPolicy", + () => loadOperationalLocalPathPolicy(meshRoot), + ); + workspaceRoot = initialPolicy.workspaceRoot; + + await operationalLogger.info("weave.started", "Starting local weave", { + workspaceRoot, + targets: options.request?.targets ?? [], + }); + await auditLogger.record("weave.started", "Local weave started", { + workspaceRoot, + targets: options.request?.targets ?? [], + }); + // existing body... } catch (error) { status = "failed";🤖 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/runtime/weave/weave.ts` around lines 680 - 699, Move the try block to start before the startup operations so failures in createRuntimeTiming("weave"), resolveLoggers(options), resolveExecutionMeshRoot(options), loadOperationalLocalPathPolicy(meshRoot) and the initial operationalLogger.info / auditLogger.record calls are caught and handled; specifically, wrap the code that assigns timing, status, resolves loggers/meshRoot, calls loadOperationalLocalPathPolicy, sets workspaceRoot/wovenDesignatorPaths, and emits the "weave.started" logs inside the try so that on error you still run the existing weave.failed logging and ensure timing.finish() is always invoked in the corresponding finally block.
🧹 Nitpick comments (2)
documentation/notes/release-notes.v0.1.2.md (1)
1-9: ⚡ Quick winRelease notes file is empty and should be populated.
This file contains only YAML frontmatter with no release notes content. Before the v0.1.2 release is finalized, this file should be populated with the actual release notes documenting the changes in this version.
Do you want me to help draft release notes content based on the changes in this PR?
🤖 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 `@documentation/notes/release-notes.v0.1.2.md` around lines 1 - 9, The release-notes file with title "Weave release notes v0.1.2" (id: baffu3p06ydec5m0eikqklb) contains only YAML frontmatter and must be populated with the actual release notes; update the document by adding a concise summary of v0.1.2, list of key features/changes, bug fixes, breaking/upgrade notes (if any), notable PRs/issue numbers and contributors, and a short migration or compatibility note, pulling descriptions from this PR and related commits to ensure each bullet references the appropriate PR/issue numbers.src/runtime/weave/pages.ts (1)
1034-1038: ⚡ Quick winRender repository URL for floating sources
Line 1034 currently exposes only
repositoryPathFromRoot. Please also renderrepositoryUrlso floating source provenance remains unambiguous even when no working URL is present.Proposed diff
if (page.repositorySourceFloatingLocator) { + rows.push({ + label: "Repository", + href: page.repositorySourceFloatingLocator.repositoryUrl, + value: page.repositorySourceFloatingLocator.repositoryUrl, + }); rows.push({ label: "Repository Source", value: page.repositorySourceFloatingLocator.repositoryPathFromRoot, }); }🤖 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/runtime/weave/pages.ts` around lines 1034 - 1038, The code only adds repositoryPathFromRoot for floating sources via the rows.push call when page.repositorySourceFloatingLocator exists; update this to also render repositoryUrl so provenance is unambiguous—either add a second row (label "Repository URL", value page.repositorySourceFloatingLocator.repositoryUrl) or include repositoryUrl alongside repositoryPathFromRoot in the existing "Repository Source" value, ensuring you reference page.repositorySourceFloatingLocator and the existing rows.push invocation to modify the pushed object.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@documentation/notes/wd.general-guidance.md`:
- Line 23: The live-server command in the documentation has a typo in the mount
argument for mesh-branch-fantasy-rules: replace the double-slash protocol-like
sequence `--mount=/mesh-branch-fantasy-rules://home/...` with the correct mount
syntax using a single colon and single slash (e.g.,
`--mount=/mesh-branch-fantasy-rules:/home/...`) so the full command (the
`live-server ... --mount=...` line) uses consistent `--mount=<name>:<path>`
entries like the other mounts.
In `@src/core/weave/weave_test.ts`:
- Around line 2608-2626: Test only mutates input.currentMeshInventoryTurtle but
leaves input.referenceTargetSourcePayloadArtifact as the original working-path
payload, so it never actually exercises the floating-source code path; update
the test to also replace or rebuild input.referenceTargetSourcePayloadArtifact
to contain a floating RepositorySourceFloatingLocator (matching the change made
to currentMeshInventoryTurtle) before calling planWeave, or use a helper that
returns an input whose referenceTargetSourcePayloadArtifact already uses the
floating locator; reference the test symbols createExtractedBobWeaveInput,
input.referenceTargetSourcePayloadArtifact, input.currentMeshInventoryTurtle,
and planWeave when making this change.
In `@src/core/weave/weave.ts`:
- Around line 1116-1118: The createdPages emitted by
buildFirstPayloadWeavePages() currently omit the new floating-source metadata,
so freshly woven identifier pages lack
workingAccessUrl/repositorySourceFloatingLocator; update the
IdentifierResourcePageModel construction (and the identifierPage helper if
present) to accept and set workingLocalRelativePath, workingAccessUrl, and
repositorySourceFloatingLocator, then thread
payloadArtifact.repositorySourceFloatingLocator and the computed
workingAccessUrl into each createdPages entry where identifier pages are
produced (see call sites around buildFirstPayloadWeavePages(), identifierPage,
and places emitting "identifier" kind pages) so the rendered Turtle can access
these fields without requiring a reparse.
- Around line 6341-6366: The assertHasCurrentSourceLocatorPath check currently
treats any floating locator with the same sourceRepositoryPathFromRoot as a
match; instead, plumb the RepositorySourceFloatingLocator through the
extracted-source artifact model and replace the loose path-only comparison in
hasRepositorySourceFloatingLocatorPathFact with the stricter locator-equality
check used elsewhere (i.e., compare both sourceRepositoryUrl and path and handle
duplicate locator nodes), reuse the existing locator equality utility rather
than matching only sourceRepositoryPathFromRoot, and ensure calls to
normalizeWorkingLocalRelativePathLiteral are wrapped so any thrown Error is
caught and rethrown as a WeaveInputError; keep hasCurrentWorkingFileLocator
usage but only return success when the full RepositorySourceFloatingLocator
matches.
In `@src/runtime/operational/local_path_policy.ts`:
- Around line 320-327: collectRepositorySourceCandidateRoots is currently adding
mesh-derived rule roots even when they lie outside policy.workspaceRoot; update
the loop that handles rule.locatorKinds.includes("workingLocalRelativePath") so
that after computing root = resolveRuleRoot(policy, rule) you verify the
resolved root is inside the workspace by using the same guard as
resolveAllowedLocalPath (or compare against policy.workspaceRoot) before adding
to roots; only add resolve(root) into the roots set when the workspace-root
check passes to prevent floating-locator resolution outside the intended
workspace.
---
Outside diff comments:
In `@src/runtime/weave/weave.ts`:
- Around line 1694-1750: The current-source loading path stops honoring
payloadArtifact.repositorySourceFloatingLocator because code always calls
resolveAllowedLocalPath for the canonical/raw source files; update the branches
that compute absolute paths for current-source/canonical references to first
check payloadArtifact.repositorySourceFloatingLocator and, when present, call
resolveRepositorySourceFloatingLocalPath(localPathPolicy,
payloadArtifact.repositorySourceFloatingLocator) instead of
resolveAllowedLocalPath; ensure the same logic is applied wherever
resolveAllowedLocalPath is used for working/canonical source resolution (e.g.,
the block around readTextFileWithOverlay that sets currentPayloadTurtle and the
similar block at lines ~2857-2865), and keep throwing the same
LocalPathAccessError/Deno.errors.NotFound handling and preserve
artifactResolutionMode_working / PayloadWorkingArtifact metadata plumbing.
- Around line 680-699: Move the try block to start before the startup operations
so failures in createRuntimeTiming("weave"), resolveLoggers(options),
resolveExecutionMeshRoot(options), loadOperationalLocalPathPolicy(meshRoot) and
the initial operationalLogger.info / auditLogger.record calls are caught and
handled; specifically, wrap the code that assigns timing, status, resolves
loggers/meshRoot, calls loadOperationalLocalPathPolicy, sets
workspaceRoot/wovenDesignatorPaths, and emits the "weave.started" logs inside
the try so that on error you still run the existing weave.failed logging and
ensure timing.finish() is always invoked in the corresponding finally block.
---
Nitpick comments:
In `@documentation/notes/release-notes.v0.1.2.md`:
- Around line 1-9: The release-notes file with title "Weave release notes
v0.1.2" (id: baffu3p06ydec5m0eikqklb) contains only YAML frontmatter and must be
populated with the actual release notes; update the document by adding a concise
summary of v0.1.2, list of key features/changes, bug fixes, breaking/upgrade
notes (if any), notable PRs/issue numbers and contributors, and a short
migration or compatibility note, pulling descriptions from this PR and related
commits to ensure each bullet references the appropriate PR/issue numbers.
In `@src/runtime/weave/pages.ts`:
- Around line 1034-1038: The code only adds repositoryPathFromRoot for floating
sources via the rows.push call when page.repositorySourceFloatingLocator exists;
update this to also render repositoryUrl so provenance is unambiguous—either add
a second row (label "Repository URL", value
page.repositorySourceFloatingLocator.repositoryUrl) or include repositoryUrl
alongside repositoryPathFromRoot in the existing "Repository Source" value,
ensuring you reference page.repositorySourceFloatingLocator and the existing
rows.push invocation to modify the pushed object.
🪄 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: 6602420d-a371-4edf-aeff-7ecea8608bb1
📒 Files selected for processing (33)
.github/workflows/ci.ymldendron.ymldocumentation/notes/release-notes.v0.1.2.mddocumentation/notes/wd.codebase-overview.caching.mddocumentation/notes/wd.general-guidance.mddocumentation/notes/wd.todo.mddocumentation/notes/wu.cli-reference.examples.sflo.mddocumentation/notes/wu.cli-reference.integrate.mddocumentation/notes/wu.repository-options.mdsrc/cli/run.tssrc/core/extract/extract.tssrc/core/extract/extract_test.tssrc/core/integrate/integrate.tssrc/core/integrate/integrate_test.tssrc/core/weave/weave.tssrc/core/weave/weave_test.tssrc/runtime/extract/extract.tssrc/runtime/integrate/integrate.tssrc/runtime/mesh/inventory.tssrc/runtime/mesh/inventory_test.tssrc/runtime/operational/local_path_policy.tssrc/runtime/operational/local_path_policy_test.tssrc/runtime/timing.tssrc/runtime/weave/pages.tssrc/runtime/weave/pages_test.tssrc/runtime/weave/weave.tstests/e2e/integrate_cli_test.tstests/e2e/weave_cli_test.tstests/integration/branch_fantasy_rules_fixture_test.tstests/integration/defaults_guardrails_test.tstests/integration/extract_test.tstests/integration/weave_test.tsweave.code-workspace
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
|
|
||
| It's nice to have live-server running for checking meshes: | ||
|
|
||
| `live-server /home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow --port=4507 --mount=/sflo:/tmp/sflo/ --mount=/mesh-alice-bio:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/sflo --mount=/mesh-sidecar-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-sidecar-fantasy-rules/docs --mount=/mesh-branch-fantasy-rules://home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-branch-fantasy-rules` |
There was a problem hiding this comment.
Fix the mount path syntax.
The mount path --mount=/mesh-branch-fantasy-rules://home/djradon/... contains a double slash :// which appears to be a typo. The live-server mount syntax should use a single colon followed by a single slash.
🔧 Proposed fix
-`live-server /home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow --port=4507 --mount=/sflo:/tmp/sflo/ --mount=/mesh-alice-bio:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/sflo --mount=/mesh-sidecar-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-sidecar-fantasy-rules/docs --mount=/mesh-branch-fantasy-rules://home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-branch-fantasy-rules`
+`live-server /home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow --port=4507 --mount=/sflo:/tmp/sflo/ --mount=/mesh-alice-bio:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/sflo --mount=/mesh-sidecar-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-sidecar-fantasy-rules/docs --mount=/mesh-branch-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-branch-fantasy-rules`📝 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
| `live-server /home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow --port=4507 --mount=/sflo:/tmp/sflo/ --mount=/mesh-alice-bio:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/sflo --mount=/mesh-sidecar-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-sidecar-fantasy-rules/docs --mount=/mesh-branch-fantasy-rules://home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-branch-fantasy-rules` | |
| `live-server /home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow --port=4507 --mount=/sflo:/tmp/sflo/ --mount=/mesh-alice-bio:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/sflo --mount=/mesh-sidecar-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-sidecar-fantasy-rules/docs --mount=/mesh-branch-fantasy-rules:/home/djradon/hub/semantic-flow/weave/dependencies/github.com/semantic-flow/mesh-branch-fantasy-rules` |
🤖 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 `@documentation/notes/wd.general-guidance.md` at line 23, The live-server
command in the documentation has a typo in the mount argument for
mesh-branch-fantasy-rules: replace the double-slash protocol-like sequence
`--mount=/mesh-branch-fantasy-rules://home/...` with the correct mount syntax
using a single colon and single slash (e.g.,
`--mount=/mesh-branch-fantasy-rules:/home/...`) so the full command (the
`live-server ... --mount=...` line) uses consistent `--mount=<name>:<path>`
entries like the other mounts.
Comment on lines
+2608
to
+2626
| Deno.test("planWeave accepts extracted terms from floating repository source payloads", async () => { | ||
| const input = await createExtractedBobWeaveInput(); | ||
| input.currentMeshInventoryTurtle = input.currentMeshInventoryTurtle.replace( | ||
| "sflo:hasWorkingLocatedFile <alice-bio.ttl> ;", | ||
| `sflo:hasRepositorySourceFloatingLocator [ | ||
| a sflo:RepositorySourceFloatingLocator ; | ||
| sflo:sourceRepositoryUrl "https://github.com/semantic-flow/mesh-alice-bio.git" ; | ||
| sflo:sourceRepositoryPathFromRoot "alice-bio.ttl" | ||
| ] ;`, | ||
| ); | ||
|
|
||
| const plan = planWeave(input); | ||
|
|
||
| assertEquals(plan.wovenDesignatorPaths, ["bob"]); | ||
| assertStringIncludes( | ||
| plan.updatedFiles[0]?.contents ?? "", | ||
| "sflo:hasRepositorySourceFloatingLocator [", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test doesn't exercise the floating extracted-source path.
It only rewrites input.currentMeshInventoryTurtle; referenceTargetSourcePayloadArtifact still comes from createExtractedBobWeaveInput() with the original working-path-based payload. So the test can pass without verifying the floating-locator case named in the title.
🤖 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/core/weave/weave_test.ts` around lines 2608 - 2626, Test only mutates
input.currentMeshInventoryTurtle but leaves
input.referenceTargetSourcePayloadArtifact as the original working-path payload,
so it never actually exercises the floating-source code path; update the test to
also replace or rebuild input.referenceTargetSourcePayloadArtifact to contain a
floating RepositorySourceFloatingLocator (matching the change made to
currentMeshInventoryTurtle) before calling planWeave, or use a helper that
returns an input whose referenceTargetSourcePayloadArtifact already uses the
floating locator; reference the test symbols createExtractedBobWeaveInput,
input.referenceTargetSourcePayloadArtifact, input.currentMeshInventoryTurtle,
and planWeave when making this change.
Comment on lines
1116
to
1118
| payloadArtifact.workingLocalRelativePath, | ||
| payloadArtifact.repositorySourceFloatingLocator, | ||
| { knopMetadataHistoryPolicy, knopInventoryHistoryPolicy }, |
There was a problem hiding this comment.
Thread the new floating-source metadata into createdPages.
The new locator is only forwarded into the Turtle renderers here. buildFirstPayloadWeavePages() still emits an IdentifierResourcePageModel with just workingLocalRelativePath, so freshly woven identifier pages cannot render workingAccessUrl / repositorySourceFloatingLocator until something reparses the inventory.
Suggested wiring
createdPages: buildFirstPayloadWeavePages(
designatorPath,
payloadLayout,
payloadArtifact.workingLocalRelativePath,
+ payloadArtifact.workingAccessUrl,
+ payloadArtifact.repositorySourceFloatingLocator,
meshInventoryProgression,
{ knopMetadataHistoryPolicy, knopInventoryHistoryPolicy },
),function identifierPage(
path: string,
designatorPath: string,
workingLocalRelativePath?: string,
workingAccessUrl?: string,
repositorySourceFloatingLocator?: RepositorySourceFloatingLocator,
): IdentifierResourcePageModel {
return {
kind: "identifier",
path,
designatorPath,
workingLocalRelativePath,
workingAccessUrl,
repositorySourceFloatingLocator,
};
}Also applies to: 1132-1134
🤖 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/core/weave/weave.ts` around lines 1116 - 1118, The createdPages emitted
by buildFirstPayloadWeavePages() currently omit the new floating-source
metadata, so freshly woven identifier pages lack
workingAccessUrl/repositorySourceFloatingLocator; update the
IdentifierResourcePageModel construction (and the identifierPage helper if
present) to accept and set workingLocalRelativePath, workingAccessUrl, and
repositorySourceFloatingLocator, then thread
payloadArtifact.repositorySourceFloatingLocator and the computed
workingAccessUrl into each createdPages entry where identifier pages are
produced (see call sites around buildFirstPayloadWeavePages(), identifierPage,
and places emitting "identifier" kind pages) so the rendered Turtle can access
these fields without requiring a reparse.
Comment on lines
+6341
to
+6366
| function assertHasCurrentSourceLocatorPath( | ||
| quads: readonly Quad[], | ||
| meshBase: string, | ||
| errorMessage: string, | ||
| subjectValue: string, | ||
| workingLocalRelativePath: string, | ||
| ): void { | ||
| if ( | ||
| hasCurrentWorkingFileLocator( | ||
| quads, | ||
| meshBase, | ||
| subjectValue, | ||
| workingLocalRelativePath, | ||
| ) || | ||
| hasRepositorySourceFloatingLocatorPathFact( | ||
| quads, | ||
| meshBase, | ||
| subjectValue, | ||
| workingLocalRelativePath, | ||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| throw new WeaveInputError(errorMessage); | ||
| } |
There was a problem hiding this comment.
Match the full floating locator here, not just sourceRepositoryPathFromRoot.
This extracted-source check currently treats any floating locator with the same path-from-root as a match. That ignores sourceRepositoryUrl, ignores duplicate locator nodes, and normalizeWorkingLocalRelativePathLiteral() can escape from hasRepositorySourceFloatingLocatorPathFact() as a raw Error. In a mesh that carries two repositories with the same relative file path, this can validate against the wrong source artifact.
Plumb RepositorySourceFloatingLocator through the extracted-source artifact model and reuse the stricter locator equality check here, while wrapping normalization failures into WeaveInputError.
Also applies to: 6592-6621
🤖 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/core/weave/weave.ts` around lines 6341 - 6366, The
assertHasCurrentSourceLocatorPath check currently treats any floating locator
with the same sourceRepositoryPathFromRoot as a match; instead, plumb the
RepositorySourceFloatingLocator through the extracted-source artifact model and
replace the loose path-only comparison in
hasRepositorySourceFloatingLocatorPathFact with the stricter locator-equality
check used elsewhere (i.e., compare both sourceRepositoryUrl and path and handle
duplicate locator nodes), reuse the existing locator equality utility rather
than matching only sourceRepositoryPathFromRoot, and ensure calls to
normalizeWorkingLocalRelativePathLiteral are wrapped so any thrown Error is
caught and rethrown as a WeaveInputError; keep hasCurrentWorkingFileLocator
usage but only return success when the full RepositorySourceFloatingLocator
matches.
Comment on lines
+320
to
+327
| for (const rule of policy.rules) { | ||
| if (!rule.locatorKinds.includes("workingLocalRelativePath")) { | ||
| continue; | ||
| } | ||
| const root = resolveRuleRoot(policy, rule); | ||
| if (root) { | ||
| roots.add(resolve(root)); | ||
| } |
There was a problem hiding this comment.
Reapply the workspace-root guard for mesh-derived candidate roots.
collectRepositorySourceCandidateRoots currently includes mesh-sourced rule roots even when they resolve outside policy.workspaceRoot. That bypasses the boundary enforced in resolveAllowedLocalPath and can allow floating-locator resolution outside intended workspace scope (Line 320 onward).
🔧 Suggested fix
function collectRepositorySourceCandidateRoots(
policy: OperationalLocalPathPolicy,
): readonly string[] {
const roots = new Set<string>();
for (const rule of policy.rules) {
if (!rule.locatorKinds.includes("workingLocalRelativePath")) {
continue;
}
const root = resolveRuleRoot(policy, rule);
if (root) {
+ if (
+ rule.source === "mesh" &&
+ !isWithinRoot(root, policy.workspaceRoot)
+ ) {
+ continue;
+ }
roots.add(resolve(root));
}
}
roots.add(resolve(policy.meshRoot));
return [...roots];
}🤖 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/runtime/operational/local_path_policy.ts` around lines 320 - 327,
collectRepositorySourceCandidateRoots is currently adding mesh-derived rule
roots even when they lie outside policy.workspaceRoot; update the loop that
handles rule.locatorKinds.includes("workingLocalRelativePath") so that after
computing root = resolveRuleRoot(policy, rule) you verify the resolved root is
inside the workspace by using the same guard as resolveAllowedLocalPath (or
compare against policy.workspaceRoot) before adding to roots; only add
resolve(root) into the roots set when the workspace-root check passes to prevent
floating-locator resolution outside the intended workspace.
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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.
@CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
WEAVE_TIMINGenvironment variable.Documentation
weave integratecommand.Chores