Close remaining D-root helper and cleanup gaps - #313
Conversation
|
@codex review please. Inspect the complete PR and comment all findings possible in one review pass. Check current-vs-historical C-root classification, canonical D-root routing, helper defaults, validator/fixture behavior, packet/ZIP cleanup semantics, source-owner registration, merge-stable source truth, and any security or path-confinement issue. Do not limit review to the latest resolution or a single commit. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a628831f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review please. Inspect the complete updated PR and report all remaining findings in one pass. Verify every prior C-root classification, D-root route, helper default, packet/ZIP cleanup rule, parity manifest and cleanup preflight, reparse-point handling, timestamp freshness, structured error behavior, source-owner registration, fixture coverage, and merge-stable source truth. Review the full PR, not only the latest commit or this resolution. |
|
@codex review please. Review the complete current PR at its current head and report all remaining findings in one pass. Check D-root routing, historical/current C-root classification, cleanup parity manifest, packet and ZIP freshness/parity, reparse-point and partial-deletion safety, structured failure paths, source ownership, fixtures, and merge-stable truth. Do not limit review to this comment or one commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c530fde317
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if HISTORICAL_WORDS.search(line) or HISTORICAL_WORDS.search(context): | ||
| return "historical" |
There was a problem hiding this comment.
Do not treat broad words such as fixture, rollback, migration, cleanup, or rejected as sufficient evidence that a route is historical. The earlier spelling gap is fixed, but fresh evidence remains in dev/orin_user_review_bundle.py:7582-7616: executable packet templates containing current C-root commands are matched and then classified as historical because those ordinary planning words occur on the same line or in the preceding context. Consequently, the repository scan reports zero current findings while active generated packets still direct commands and outputs to the superseded roots.
Useful? React with 👍 / 👎.
| if not fixture_mode and ("fixtures" in path.parts or "branch_records" in path.parts): | ||
| return "historical" |
There was a problem hiding this comment.
Do not classify the entire branch_records tree as historical, because Docs/branch_records/index.md is live routing law rather than a retired receipt. Fresh evidence after narrowing the branch-plan exemption is that current rules at index lines 56 and 60 still route operational state to C:\Nexus Governance State, yet this directory-wide return labels those occurrences historical and lets the validator exit successfully. Exempt individual historical records while continuing to scan the router and other current owners.
Useful? React with 👍 / 👎.
| for target in SUPERSEDED_ROOTS: | ||
| if target.exists(): | ||
| shutil.rmtree(target) |
There was a problem hiding this comment.
When Windows refuses a later rmtree because a file becomes locked, an ACL denies deletion, or another filesystem error occurs after the first root is removed, this loop irreversibly leaves a partial cleanup and escapes without the promised JSON report. Fresh evidence after adding the all-target preflight is that the destructive calls themselves remain unguarded; moreover, retrying is then blocked because relocation_parity() requires the already-deleted source root to exist. Handle destructive failures as structured results and provide a recovery-safe deletion strategy before removing the first root.
Useful? React with 👍 / 👎.
| if not receipt.is_file(): | ||
| return | ||
| text = receipt.read_text(encoding="utf-8", errors="ignore") |
There was a problem hiding this comment.
When the audit receipt exists but cannot be read due to an ACL, transient lock, or I/O failure, this unguarded read_text() raises before verify() can return its structured refusal report; the second receipt read around line 221 has the same problem. The archive hashing paths now catch unreadable ZIP failures, but an unreadable receipt still produces a traceback instead of a stable nonzero validation result explaining why cleanup was blocked.
Useful? React with 👍 / 👎.
| try: | ||
| generated = dt.datetime.fromisoformat(marker.group(1).strip().replace("Z", "+00:00")) | ||
| if generated.astimezone(dt.timezone.utc).strftime("%Y%m%d-%H%M%S") != archive.stem.removeprefix("Governance-"): | ||
| failures.append("packet ZIP timestamp does not match receipt generation timestamp") |
There was a problem hiding this comment.
Reject a sole Governance archive whose valid timestamp is stale or future-dated rather than merely checking that its self-authored receipt contains the same timestamp. Fresh evidence after invalid calendar dates were rejected is that this comparison still has no current-time, relocation-run, or authoritative generation-event anchor, so an old packet and receipt can pass whenever they are the only siblings and the underlying data has not changed; a future-dated matching pair also passes. Such a packet can still authorize destructive cleanup despite the stated stale-ZIP refusal contract.
Useful? React with 👍 / 👎.
| external = Path( | ||
| r"C:\Nexus Governance State\branches\feature_fam_006_dashboard_recording_start_stop_local_file\branch_plan.md" | ||
| r"D:\Nexus Desktop AI Data\Governance State\branches\feature_fam_006_dashboard_recording_start_stop_local_file\branch_plan.md" | ||
| ) | ||
| external_state = Path( | ||
| r"C:\Nexus Governance State\branches\feature_fam_006_dashboard_recording_start_stop_local_file\branch_state.md" | ||
| r"D:\Nexus Desktop AI Data\Governance State\branches\feature_fam_006_dashboard_recording_start_stop_local_file\branch_state.md" | ||
| ) |
There was a problem hiding this comment.
Build these active branch-plan and branch-state paths from EXTERNAL_STATE_ROOT rather than embedding the D-root literal. When the canonical root map changes or a clean-room/replay run supplies another root, this helper's USER_ROOT follows nexus_paths while source_truth_loaded_lines() and the repeated packet-copy sites continue reading the old D location, silently reporting the current external source truth missing and omitting it from generated packets.
Useful? React with 👍 / 👎.
| "beforeAfterDesktopComparisonReady", | ||
| "PrepareLiveValidationUserTestSummary", | ||
| 'UserTestSummary = "C:\\Nexus USER\\UTS - FAM-006.txt"', | ||
| f'UserTestSummary = "{USER_HUB_ROOT}\\UTS - FAM-006.txt"', |
There was a problem hiding this comment.
Use a Windows-path renderer rather than interpolating the host-native Path representation into this source check. On a POSIX clean clone, USER_HUB_ROOT renders as D:\Nexus Desktop AI Data/USER, so this validator searches for UserTestSummary = "D:\Nexus Desktop AI Data/USER\UTS - FAM-006.txt" while the PowerShell helper correctly contains all backslashes; running the validator in the current environment consequently reports this required marker missing. The same interpolation pattern can also emit mixed-separator paths into generated packets.
Useful? React with 👍 / 👎.
| scan_root = Path(args.root).resolve() | ||
| findings = scan(scan_root / "dev") + scan(scan_root / "Docs") | ||
| fixture_failures = run_fixture_tests() if args.run_fixture_tests else [] |
There was a problem hiding this comment.
Pass scan_root into the fixture runner instead of always loading fixtures from the checkout containing this script. When --root points at a clean-room clone or another candidate checkout, the main scan reads that target but --run-fixture-tests silently tests ROOT/dev/fixtures from the original checkout, so missing or regressed fixtures in the selected repository can still be reported green.
Useful? React with 👍 / 👎.
| for source_file in sorted(source_root.rglob("*")): | ||
| if not source_file.is_file(): | ||
| continue |
There was a problem hiding this comment.
Treat directory traversal failures as parity failures rather than relying on Path.rglob() to prove a complete source inventory. On Python versions that suppress filesystem scan errors, an ACL-protected nested directory can be omitted without incrementing unreadable_count or adding a failure, allowing the manifest to report PASS despite unverified source files; Windows ACLs can permit deletion independently of listing/read access, so cleanup may then remove data that was never compared with D. Enumerate directories with explicit error handling and bind traversal completeness into the proof.
Useful? React with 👍 / 👎.
| manifest = Path(args.relocation_manifest).resolve() | ||
| if args.write_relocation_manifest: | ||
| manifest.parent.mkdir(parents=True, exist_ok=True) | ||
| manifest.write_text( |
There was a problem hiding this comment.
Check that the requested manifest path is inside the canonical Governance packet before creating directories or writing any bytes. With --write-relocation-manifest, a typo or incorrectly supplied absolute path currently causes this code to create parent directories and overwrite that file before verify() notices the path is outside the packet and reports failure; because write_text() replaces existing content, the supposedly guarded helper can corrupt an unrelated file even though cleanup itself remains blocked.
Useful? React with 👍 / 👎.
1 participant
Summary
Scope
Governance/source-truth/helper/validator/fixture and relocation closure only. No product/FAM, runtime/provider/private/cache/memory, issue, release, or unrelated PR mutation.
Validation
python -B -m compileall -q devpython -B dev/orin_workspace_root_residue_validation.py --run-fixture-testspython -B dev/orin_source_owner_marker_validation.pypython -B dev/orin_branch_governance_validation.pypython -B dev/orin_branch_governance_validation.py --worktree-confinement-gatepython -B dev/orin_branch_governance_validation.py --release-readiness-health-gatepython -B dev/orin_branch_readiness_planning_fixture_validation.pypython -B dev/orin_governance_efficiency_validation.pypython -B dev/orin_external_state_validation.py --root D:\Nexus Desktop AI Data\Governance State --repo D:\Nexus Desktop AI Data\Worktrees\Governance-Final-Closurepython -B dev/orin_public_leak_prevention_validation.pypython -B dev/orin_ai_provider_state_validation.pypython -B dev/orin_release_body_validation.pygit diff --check