feat: add architecture semantic coverage checks - #239
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
Thanks for the semantic coverage contribution. Reviewed exact head a9e1f8b. Changes are needed to make a passing coverage receipt auditable and to preserve the published package contract.
Spec
Satisfied requirements disappear from the receipt; missing self-paths incorrectly pass; whitespace-only omission reasons suppress missing facts. See inline reproductions.
Standards / delivery
The new diagnostic helper returns only severity/code/message/subject; please include the documented concrete evidence and executable supportedFixes rather than making agents scrape prose. Also rebuild archify.zip canonically: after the intended package.json cleanup, 74 of 77 payload files differ from tracked bytes because the archive uses CRLF and the index/source uses LF (e.g. LICENSE 1168 vs1146 bytes). Normalizing those line endings makes the texts equal, but the committed package is not a byte-reproducible build of tracked source.
Verification
New semantic coverage plus geometry tests: 60 passed, 0 failed. Additional schema/coverage reproductions confirm the findings; archive checked member-by-member. No browser/manual-visual or full-suite pass claimed; no source edits or merge.
| } | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
[P2] Include the represented requirements in the coverage receipt. On components api/worker with api->worker, an empty checklist and a checklist requiring worker, the direct edge, and the path generate the exact same pass/0-warning/empty-diagnostics receipt. #237 requires represented/missing/explicitly-omitted evidence, but satisfied subjects are skipped above and never serialized; a downstream reader cannot tell what was checked. Persist checked subjects and per-requirement outcomes in the artifact and checker receipt.
| } | ||
|
|
||
| function hasDirectedPath(adjacency, from, to) { | ||
| if (from === to) return true; |
There was a problem hiding this comment.
[P2] Confirm path endpoints exist before accepting a zero-length path. evaluateArchitectureSemanticCoverage({components:[{id:'api'}],connections:[],semanticChecks:{requiredPaths:[{from:'missing',to:'missing'}]}}) currently returns pass with no diagnostics because from===to returns immediately. No such component/path exists. Preserve valid self-reachability only for existing authored components and add this missing-endpoint regression.
| "properties": { | ||
| "kind": { "const": "component" }, | ||
| "id": { "$ref": "common.schema.json#/$defs/id" }, | ||
| "reason": { "type": "string", "minLength": 1 } |
There was a problem hiding this comment.
[P2] Reject whitespace-only omission reasons. minLength:1 accepts reason:' '; for requiredComponents:['scheduler'] with no scheduler, adding that omission passes schema validation and changes semanticCoverage from warn to pass with zero warnings. The same reason schema occurs for edge/path/external-label omissions. Require non-whitespace content in both source/generated validation and add a regression so suppressions satisfy the reasoned-omission contract.
a9e1f8b to
12f6dae
Compare
|
Addressed the requested changes in
The focused regression suite passes 7/7, the skill validator passes, and the archive entries match the committed sources. The full local suite is constrained by Windows symlink/Unix-mode/Bash environment failures; please use the Linux CI result for the complete gate and re-review when ready. |
tt-a1i
left a comment
There was a problem hiding this comment.
Reviewed current head 12f6daedf15d0c20a0fccbd0668c614564049029. The prior receipt-ledger, missing self-path, whitespace omission, structured-diagnostic, and ZIP byte issues are fixed. One core scope gap still prevents this PR from closing #237 as written.
Spec — derive discovered repository facts instead of requiring authors to restate every omission
Issue #237 asks for semanticCoverage over inferred repository relationships and specifically says a discovered lifecycle scheduler or background worker that is absent from the diagram should warn. The implementation explicitly defines coverage as an “explicit authoring contract, not repository inference” (archify/renderers/architecture/semantic-coverage.mjs:97-102) and returns null whenever the author did not already add semanticChecks.
This reproduces through the public CLI. I created a real Git repository containing tracked src/api.js and src/scheduler.js, pinned the diagram's meta.repository to that exact revision, referenced only src/api.js, omitted the scheduler from the diagram, and ran:
archify validate architecture input.json --repo-root <repo> --json
It exits 0 with ok: true, all 9 artifact checks passing, composition.status: "pass", 0 warnings, no issues, and no semanticCoverage field. In other words, the feature can audit only facts an author already knows and duplicates into required*; it cannot catch the issue's primary failure mode—an important repository fact the author did not know they omitted.
Keep semanticChecks as the ledger if useful, but populate or verify it from a repository-discovery result (or require a completeness-checked evidence manifest) so the fixture above emits a non-blocking scheduler warning. Add that public --repo-root regression. If the intended scope is only an author-supplied checklist validator, narrow the claim and do not close #237 without an explicit maintainer decision.
Verification on a synthetic merge with current main 199360cc6687a7857b54dd188d4922b09e466a4b: full suite 998 passed, 0 failed, 31 skipped; focused semantic/CLI/delivery suite 53/53; git diff --check and generated-validator freshness pass. The archive hash matches the PR description and all 77 canonical payload members match source. These green checks confirm the implementation is internally consistent, but do not cover the missing discovery path above. Current remote head has no CI and is behind main; those remain separate merge gates.
Add opt-in, non-blocking coverage diagnostics for required components, edges, paths, and external relationship labels. Include reasoned omissions, artifact receipts, authoring guidance, regression tests, and the rebuilt skill archive.\n\nRefs tt-a1i#237
12f6dae to
b735a95
Compare
|
Addressed the current review on the rebased head
Validation completed: generated-validator freshness; focused semantic-coverage and repository-evidence tests; Node 22 deterministic ZIP rebuild; and extracted-package smoke on Windows. The full runner also exposes pre-existing upstream snapshot/environment failures in unchanged files ( |
|
感谢你为这个 PR 投入的大量时间,也感谢你认真处理了多轮审核意见。其中可审计清单、结构化诊断和显式省略理由等设计很有价值。 |
Summary
Adds an opt-in
semanticCheckscontract for repository-backed architecture diagrams and incorporates the requested review fixes.mainand rebuilds the checked-in archive canonically with Node 22.Regenerated files
archify/renderers/shared/generated-validators.mjsarchify.zipValidation
node --test test/architecture-semantic-coverage.test.mjs— 7/7 passed on Node 22;quick_validate.py— passed;scripts/build-zip.shrun twice — identical 77-file archive, SHA-2565a5a6553cf3dae429cbb931e9d05c89b1461544e72a6b2679ce1a80f84360d9b;git diff --check— passed.The full repository suite was also exercised locally; Windows-only environment limitations around symlink creation, Unix file modes, and Bash resolution prevent a clean local result, so the Linux CI run remains authoritative for the complete suite.
Closes #237