Repository navigation
review: say module-relative where the lane IS module-relative (#3514 follow-up) - #3539
Merged
Merged
Conversation
…follow-up) Three Copilot findings on #3535, all message/comment drift with no behaviour, landing separately because they arrived after auto-merge had already enqueued that PR and a push would have ejected it. * `test-module-asset-landing.py`'s `LANES` comment described a `(workflow, job id, step id)` tuple for a `(workflow, step id)` one. Extraction is by step id alone, across every job in the file — which is the property that makes it survive a job rename, so the comment was understating the guarantee as well as being wrong. * Both lanes' summary line said `landed under /ext/<module>/wwwroot` while the copy is deliberately `moduleassets/.` — module-relative, so a second asset root would ride too. Stating today's only root as the contract contradicts the reason the copy is written that way, and would read as a bug the day the packer emits another root. Now `landed module-relative under /ext/<module>/`, and Doc/Architecture/ModuleStaticAssets quotes the corrected line. Verified: the harness still passes both lanes' four cases with both falsification arms red; actionlint, check-workflow-shell.py, check-workflow-timeouts.py clean; MeshWeaver.Documentation.Test 304/304 in Release -warnaserror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated doc page currently splits a single inline code span across a newline, which is invalid/unstable Markdown rendering and should be formatted as a single-line code span (or a fenced block).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is a follow-up documentation/log-message clarification to #3535/#3514, aligning comments and emitted summary text with the lane’s intended contract (“module-relative” landing) without changing behavior.
Changes:
- Update the reusable workflow summary lines to say assets land module-relative under
/ext/<module>/(not specifically/wwwroot). - Fix/clarify the harness comment describing how the lane step is extracted (by step id across jobs).
- Update the
ModuleStaticAssetsdoc page to quote the updated summary line.
File summaries
| File | Description |
|---|---|
src/MeshWeaver.Documentation/Data/Architecture/ModuleStaticAssets.md |
Updates the quoted lane output text to match the updated contract wording. |
.github/workflows/node-repo-publish-bake.yml |
Adjusts the emitted “external modules …” summary line to say module-relative under /ext/<module>/. |
.github/workflows/node-repo-gate.yml |
Same summary line wording adjustment as publish-bake. |
.github/scripts/test-module-asset-landing.py |
Clarifies lane-extraction comment and updates the harness “ok” line to say module-relative under /ext/<module>/. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
144
to
+146
| The lane's own check is per file and prints a denominator: | ||
| `external modules: 30 composed, 6 of them carrying static web assets, 294 asset file(s) landed`. | ||
| `external modules: 30 composed, 6 of them carrying static web assets, 294 asset file(s) landed | ||
| module-relative under /ext/<module>/`. |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
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
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.
Follow-up to #3535 (which closed #3514). Three Copilot findings on that PR, all correct, all
message/comment drift with no behaviour change — they arrived a minute after auto-merge had
already enqueued #3535, and a push would have ejected it from the queue for three strings.
test-module-asset-landing.py'sLANEScomment described a(workflow, job id, step id)tuplefor a
(workflow, step id)one. Extraction is by step id alone, across every job in the file— the property that makes the harness survive a job rename — so the comment was understating the
guarantee as well as being wrong.
landed under /ext/<module>/wwwrootwhile the copy is deliberatelycp -R moduleassets/.— module-relative, so a second asset root would ride too. Statingtoday's only root as the contract contradicts the reason the copy is written that way, and would
read as a bug the day the packer emits another root. Now
landed module-relative under /ext/<module>/, in both lanes, in the harness's ownokline, and in the doc page that quotesit.
Verification (unchanged bar, re-run on this branch off
main):.github/scripts/test-module-asset-landing.py— 2 lanes × 4 cases, both falsification arms stillred (pre-fix body ⇒
0/6 assets land;cpremoved ⇒ the lane's own check fails closed) ·actionlintover all workflows ·check-workflow-shell.py·check-workflow-timeouts.py(71 jobs, 0 violations) ·
MeshWeaver.Documentation.Test304/304, Release-warnaserror.