fix(repo): govern committed surface only, include bench suites in CI, repair doc links - #219
Merged
Merged
Conversation
… repair doc links Repository-wide review findings resolved: 1. Naming checker vs ignored bench workspaces: the governed identifier scan recursed into implementation/*/workspace, which holds git-ignored vendored Autoware checkouts (LIDAR-, CAMERA-, VLP-, ... upstream identifiers). 44 errors appeared on any developer checkout with a built workspace and none on clean CI - environment-dependent gating. The exemption is behavioral (git check-ignore), so tracked files under a workspace stay governed. naming-conventions.md section 11 documents the rule; two behavioral tests cover the probe. 2. CI bench coverage: pytest.ini scopes the root suite to tests/, so 22 tracked implementation test files never ran in CI under the label 'full project test suite'. New tools/run_bench_unit_tests.py runs each bench suite in an isolated process (benches share bare module names; one process cross-contaminates evidence_document imports). All 278 bench unit/contract tests pass. The one environment-dependent 009C live-inheritance test now skips with an explicit reason when the git-ignored inherited workspace build output is absent (it exercises verify_runtime.py against a built bench; the bench runtime flow covers it on real campaigns). Honest labeling: unit/contract suites, not runtime evidence. 3. Documentation local links: two governed-scope documents pointed at the retired mw_variability_configuration.sysml filename (navigation-only fix; decision text untouched). One retained-evidence README used a wrong relative depth for external-media.yaml. New tools/check_markdown_links.py validates inline local links in tracked Markdown (fenced code stripped, remote/anchors skipped, generated SVGs excluded) and runs in CI; test_check_markdown_links.py covers the committed tree and fail-closed behavior. 4. The old sysml-v2-semantic-audit.md described per-PR states and a since-merged file layout while calling itself current; it is now explicitly marked a historical record with a pointer to the validation paths that own the current baseline. Verified on this exact head: check_repo, smoke, root pytest (911 passed; 4 pre-existing environment failures reproduce on pristine origin/main: 2 from sysand not installed in the local venv, 2 from installed mcp 2.0.0 vs the 1.28.1 pin), and the isolated bench runner (all suites pass).
The 009C evidence-validation suite asserts a retained-run/reviewed squash ancestry relation against the real commit graph. A depth-1 checkout lacks the delivery commit object, so the first CI run of the bench suites failed there even though the relation holds. Give the job a full checkout.
The squash-relation test verifies a repository-history invariant against the retained run head and reviewed head of squashed PR #66. Those commit objects exist only in clones that fetched the pull refs, so any CI or fresh clone (even with full branch history) hits a git error inside the acceptance check and the test fails. Probe the objects first and skip with an explicit reason; the check itself still runs from full developer clones and the same acceptance logic guards evidence validation at bench runtime.
… naming tests Review findings on PR #219: 1. tools/check_markdown_links.py only checked filesystem existence, so an existing-but-untracked local file satisfied a link, and a path escaping the repository root resolved without complaint. The checker now resolves each link (after URL decoding and fragment/query stripping), rejects paths escaping the repository root even when a file exists there, and requires the resolved target to be tracked repository content (git ls-files). Directory links (trailing slash or resolving to a directory) are valid when tracked content exists beneath them - git tracks files, not directories. Module documentation updated to state exactly this contract. Seven regression tests cover: tracked file passes, missing file fails, existing-but-untracked fails, root escape fails, URL decoding works, fragment/query stripping works, committed corpus passes, plus tracked/empty directory-link behavior. 2. The new naming tests for the ignored-workspace exemption only exercised the ignored side when a developer-built Autoware workspace happened to exist, making them vacuous on clean CI. They are replaced by an isolated temporary git fixture that deterministically proves both sides of the rule: a git-ignored untracked runtime path is excluded from the governed surface, and a tracked file under the same ignorable directory tree remains governed (its ID tokens still checked). The production probe gains an optional repository parameter (default unchanged) so the fixture exercises the real seam; no directory-name blanket exemption is introduced and git remains the sole decision maker. A real-workspace probe test remains, skipping with a reason on clean checkouts. Validated on this head: check_repo PASS, smoke PASS, markdown checker PASS (143 files), bench runner PASS (all suites), clean-env root suite 917 passed / 4 skipped with only the four pre-existing environment-only failures (local sysand absent; installed mcp 2.0.0 vs pinned 1.28.1).
…ggregate naming regression Review findings on PR #219 round 2: 1. tools/check_markdown_links.py treated git-index membership as a replacement for existence: a tracked-but-deleted (unstaged deletion) target satisfied a link. A target must now be BOTH present in the working tree AND tracked; each failure mode has its own message and regression test (tracked-but-deleted, existing-but-untracked). 2. The inline-link extractor only recognized double-quoted titles, so [missing](target.md 'title') and angle destinations escaped checking entirely. The grammar now covers plain/angle destinations and single- or double-quoted titles, with regression tests; unsupported forms remain out of scope explicitly, not silently skipped. 3. A failed `git ls-files` inventory produced a zero-file scan that reported success. Inventory errors now fail closed with the git diagnostic (GitInventoryError; main exits 1), pinned by a test that injects exit 128. 4. Root-directory links (./) matched tracked dotfiles only. Root membership now counts any tracked file; regression added. 5. Naming: new aggregate regression proves the full governed scan (check_identifier_tokens) still checks a tracked file under an ignored directory (git add -f, gitignore rule matched via --no-index). The existing tests covered the ignore helper and the text validator separately; a caller-side blanket exemption would have passed both. Production scan code is unchanged. Validated on this head: check_repo PASS, smoke PASS, markdown corpus PASS (143 files), focused suites 45 passed/1 skipped, root suite 923 passed/4 skipped with only the four pre-existing environment-only failures (mcp 2.0.0 vs pin 1.28.1; absent .sysand sysmod lib), bench runner PASS.
…ail-closed bench inventory Round-3 review findings on the round-2 checker: 1. Inline-link extraction now uses a destination scanner instead of one regex: angle destinations, bare destinations with balanced parens, backslash escapes, and double/single-quoted titles with surrounding whitespace are handled by rule. A link whose destination cannot be scanned cleanly (e.g. quoted title plus stray trailing whitespace, or a parenthetical title) is reported as unparsable instead of being silently skipped. 2. Directory links additionally require the tracked content beneath to exist in the working tree: the Git index alone no longer satisfies a link whose target directory (and children) were deleted unstaged. 3. tools/run_bench_unit_tests.py verifies the git inventory before running suites and fails closed (exit 1, diagnostic on stderr) when git ls-files fails: an unreadable inventory must not shrink the run to zero suites and report success. Validated on this head: focused suites 46 passed/1 skipped; bench runner passes all five suites; check_repo, smoke, and the 143-file Markdown corpus pass.
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
Resolves the repository-integrity findings from the whole-repo ontology and traceability review (branch review head
75d5f618, findings re-validated against currentmain):Environment-dependent naming gate — the governed identifier scan recursed into git-ignored bench runtime workspaces (
implementation/aebs-autoware-executable-bench/workspace/..., a vendored Autoware checkout). Any developer checkout with a built workspace failedcheck_repowith 44 upstream-identifier errors; a clean CI checkout passed. The exemption is behavioral (git check-ignore), so any tracked file under such a directory remains governed.docs/naming/naming-conventions.md§11 documents the rule; behavioral tests cover the probe and the committed surface.CI never ran implementation test suites —
pytest.iniscopes the root suite totests/, so 22 tracked bench test files were excluded from the step labeled "full project test suite". Newtools/run_bench_unit_tests.pyruns each bench suite in an isolated process (benches share bare module names such asevidence_document; a single process cross-contaminates imports and produces false failures — verified during review). Wired intoci.ymlwith an honest label: these are unit/contract suites, not runtime evidence.Environment-dependent 009C test made explicit —
test_runtime_verifier_accepts_live_inheritance_and_rejects_mismatchrequires the git-ignored inherited 009A workspace build output; it now skips with an explicit reason when that output is absent instead of failing on source checkouts. The real bench runtime flow (verify_runtime.pyat launch) covers this contract on actual campaigns.Documentation navigation — two governed product-line scope documents linked the retired
mw_variability_configuration.sysmlfilename (link target only; decision text and ADR history untouched). One retained-evidence README used a wrong relative depth forexternal-media.yaml. Newtools/check_markdown_links.pyvalidates inline local links across tracked Markdown (fenced code stripped, remote URLs/anchors skipped, generated SVGs excluded) and runs in CI.Stale audit labeled historical —
sysml-v2-semantic-audit.mddescribed per-PR states and a since-merged file layout while presenting itself as a current-baseline audit; it is now explicitly marked historical with pointers to the validation paths that own the current baseline.Not fixed here (resolved on main already)
The review also reported stale
run_override_profile.shtest references, obsolete exception-message expectations, and five missing 009B run artifacts. Investigation shows those were artifacts of reviewing the stalefeat/public-ask-viewercheckout: PR #188 (64d4e6a) already rewrote the runner-contract tests againstlaunch.sh+contract-009d.yaml, updated the exception expectations, and restored the four hash-bound run artifacts (git add -f; zero-byte log deliberately not restored per evidence policy). All 126 nominal-bench tests pass on currentmain.Validation evidence (exact head of this PR)
python scripts/check_repo.py— PASS (previously failed with 44 workspace-scan errors).python scripts/smoke_test.py— PASS.python tools/check_markdown_links.py— PASS (143 tracked Markdown files).python tools/run_bench_unit_tests.py— all five bench suites pass (278 tests + 98 subtests; includes the honest skip above when the inherited workspace is absent).python -m pytest -q— 911 passed, 3 skipped; 4 remaining failures reproduce identically on pristineorigin/mainand are environment-only (local venv lacks pinnedsysand; installedmcpis 2.0.0 vs the pinned 1.28.1).tests/test_check_naming.py(+2),tests/test_ci_integrity.py(+1),tests/test_check_markdown_links.py(+2).Follow-up: review-finding fixes (head 16c9555)
git ls-files); directory links are valid when tracked content exists beneath them. Seven+ regression tests (tracked/missing/untracked/escape/decode/fragment/query/corpus/directory).Exact-head result: all checks SUCCESS.
Merge ordering
This PR and PR #220 touch disjoint file sets (verified via the API), so either merge order is conflict-free. After the first merge, update the remaining branch onto the new
mainand let exact-head checks (including privileged Syside validation for #220) rerun on the combined state before merging. Both PRs remain drafts; merge decisions stay with the maintainer per standing rule.Round-2 review repairs (head 2842647)
The independent merge review found four defects in the new Markdown checker and a test-coverage gap in the naming exemption. All are fixed on this head; production naming scan code is unchanged.
git ls-filesproduced a zero-file scan that passed. Inventory errors now fail closed with the git diagnostic (GitInventoryError; exit 1), pinned by a test injecting exit 128../now counts any tracked file; regression added.check_identifier_tokens) still checks a tracked file under an ignored directory (git add -f, gitignore rule matched via--no-index). The previous tests covered the ignore helper and text validator separately, so a caller-side blanket exemption would have passed both.Validated at head 2842647: checks SUCCESS (run 34195023078, merge-ref tree identical to head tree); check_repo PASS; smoke PASS; markdown corpus PASS (143 files); focused suites 45 passed / 1 skipped; root suite 923 passed / 3 skipped / 4 environment-only failures (reproduce on pristine main); bench runner PASS.
Round-3 review repairs (head 0007557)
The second adversarial review found three more defects. All are fixed on this head.
[missing](missing.txt "title" )or a parenthetical title) is now reported as unparsable instead of silently skipped.[dir](gone/)with an indexed-but-absentgone/child.txtnow fails).tools/run_bench_unit_tests.pyverifies the git inventory up front and exits 1 with a diagnostic whengit ls-filesfails; an unreadable inventory can no longer shrink the run to zero suites and report success.Validated at head 0007557: checks SUCCESS (run 34202870427, merge-ref tree
6c3ce966identical to head tree); focused suites 46 passed / 1 skipped; bench runner passes all five suites; check_repo, smoke, and the 143-file Markdown corpus pass.