[Dong1017]

Governing Issue and delivery slice

Refs #178

• Accepted scope / delegation: the five identity findings in the Issue, plus the build_skill_index Git read it also reports.
• What this PR delivers: source identity that covers submodule contents, nested repositories and unrepresentable filenames; device qualification that requires a verified selection; cache-path guards that compare expanded paths; a hardened Git read in the skill index builder. Regression tests for each, all failing against 38d5883.
• What it does not deliver: no change to run-state truthfulness or frozen-plan integrity (filed separately), no hardware or model result, no new config or abstraction layer.

Knowledge authorship declaration

{
  "schema_version": 1,
  "head_sha": "1e76ef649873ba43ce26faaa565b791843895518",
  "human_only": false,
  "material_ai_authors": [
    {
      "tool": "Claude Code",
      "model_id": "claude-opus-5-5",
      "effort": "xhigh",
      "role": "author"
    }
  ]
}

Changed artifacts and evidence

Requirement	Actual change	Check / evidence and result	Remaining gap
Submodule edits must change the identity	infra/targets/git.py records each gitlink's own identity (ls-files --stage, filtered to 160000) and propagates its dirtiness	test_submodule_local_edit_changes_snapshot_hash — fails on 38d5883 with two identical hashes, passes here	Submodule scan is gated on .gitmodules; a gitlink without that file is covered only as an untracked directory
Nested repository must not be "unavailable"	untracked directory entries are hashed over their contents instead of opened as files	test_untracked_nested_repository_is_hashed_not_unavailable — base error was PermissionError: .../nested	Directory hash skips .git, so a nested repo's own commit is not part of the identity
Non-UTF-8 name must name the file	_decode_name raises ValueError("Untracked path is not valid UTF-8: ...") inside the caught identity scope	test_non_utf8_untracked_name_reports_the_path, test_non_utf8_name_does_not_cost_a_complete_run_its_artifacts	see correction below
Device qualification must verify selection	infra/platforms/__init__.py adds parsed_tool_facts; Ascend and ROCm require the requested IDs and model in tool output; cli.py admits only available and selection_verified	test_ascend_tool_exit_code_alone_does_not_qualify_a_device, test_rocm_tool_exit_code_alone_does_not_qualify_a_device — base returned tool_available	Token matching is exact-match against whitespace-split output, the only structure npu-smi/rocminfo guarantee
Cache path must be compared expanded	infra/environment.py applies expanduser() to the source, cache, output and every cache env var	test_tilde_cache_path_cannot_hide_inside_the_source_checkout — base raised nothing	—
Skill index must use hardened Git reads	tools/build_skill_index.py uses the GIT_READ tuple for rev-parse, ls-tree, diff and cat-file	test_replacement_refs_cannot_rewrite_indexed_source_bytes — base hashed replacement bytes	—

Correction to the Issue's non-UTF-8 finding

The Issue states that on Windows the UnicodeDecodeError "propagates out of snapshot() ... so the run ends with no evaluation.json, no status.json, and no report". That traceback-escape does not reproduce on this build. UnicodeDecodeError is a subclass of ValueError, and git.py already catches (OSError, ValueError, subprocess.TimeoutExpired), so the failure stays inside repository_identity and becomes available: False. The observed pre-fix behaviour was:

available: False
error: "'utf-8' codec can't decode byte 0xff in position 4: invalid start byte"

The real defect is therefore not a lost run but an unnamed-file codec error that gives the operator nothing to act on. This PR changes the message to name the path and leaves the control flow alone. The second path the Issue describes (atomic_write_json raising UnicodeEncodeError: surrogates not allowed) exists in artifacts.py:31-36 but was not reproduced here: Windows cannot store an invalid-UTF-8 filename — os.open with b"bad-\xff.txt" fails inside ntpath.join, and a lone-surrogate name is silently rewritten by Git for Windows to b'bad-\xef\xbf\xbd.txt'. That path is left untouched.

Checks and dependencies

• Checks actually run (result and tested scope):
  • python -m ruff check src tests tools → All checks passed!
  • tests/test_runtime.py, source-identity selection (submodule or nested or utf8 or tilde) → 5 passed
  • tests/test_inspection.py, tests/test_skill_index.py, tests/test_mlx_discovery.py → all passed
  • tests/test_performance_ceiling.py (65), tests/test_goal_checkpoint.py (44), tests/test_skill_index.py (8), tests/test_flare_adapter.py (34), tests/test_knowledge_contract.py + tests/test_knowledge_cli.py → all passed
  • Pre-fix contrast: the submodule, nested-repository, non-UTF-8, Ascend, ROCm, tilde and replacement-ref tests were each re-run against 38d5883 and failed there.
• Failed, skipped, timed-out or not-run checks (if applicable):
  • The full pytest tests/ -q run did not complete. It was executed twice on this Windows host and advanced only to ~16% in 25 minutes before the wrapper timed out; the run is dominated by subprocess-heavy campaign tests, not by this change. Results are reported per file, not as a whole-suite pass.
  • tests/test_anthropic_agent.py → 2 errors (ValueError: the environment variable is longer than 32767 characters, from pytest's PYTEST_CURRENT_TEST). Pre-existing: identical errors on 38d5883.
  • tests/test_harness_improvement.py → 8 failures (ValueError: Skill metadata must remain unchanged, tools/skill_eval_improve.py:69). Pre-existing: identical on 38d5883.
  • tests/test_module_boundaries.py::test_control_imports_need_no_inference_or_image_dependencies → ModuleNotFoundError: No module named 'inferrsi' because the package is not installed in this interpreter. Pre-existing: identical on 38d5883.
• Dependencies / shared interfaces / integration assumptions: repository_identity stays callable as the snapshot hook of run_experiment; a submodule failure still resolves to available: False rather than raising, which is what keeps the finally block from destroying terminal artifacts. tool_facts is unchanged so the CUDA path and its tests are untouched; inspect_cpu and mlx never set selection_verified: false alongside an available status, so the new CLI gate does not alter them.

Remaining work and Issue closure

• Remaining Issue requirements / follow-up: none claimed beyond the correction above; the submodule scope decision is stated explicitly in the table (gitlinks are in scope, with the .gitmodules gate noted).
• Goal-acceptance owner or accepted closure delegation: maintainer of fix: source and device identity accept evidence that cannot support it #178.
• Goal ready for acceptance? Not claimed.

Handoff (only when an actual handoff or decision changed)

• Accepted decision / status and link: none; this PR proposes behaviour, it does not record an accepted decision.
• Recipient and communication status: not applicable.
• Application status and artifact/evidence: not applicable.
• Relevant workspace state and next action: branch Dong1017:fix/issue178-source-identity at 1e76ef649873ba43ce26faaa565b791843895518, based on 38d5883.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-27T12:48:41.637794Z We couldn’t complete this review. Comment @codex review to try again.	1e76ef6	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Dong1017]

Reviewed head 1e76ef649873ba43ce26faaa565b791843895518, base 5c6a8f6af7ecd8fa71cedd31889820eb1546aa50, against #178 and affected runner/inspection callers. Three correctness blockers below; no LGTM yet.

flowchart LR
  A[Source and device evidence] --> B[Identity and selection checks]
  B --> C[Run admission and final artifacts]
  D[Unavailable child / omitted symlink / unrelated output token] -. current gaps .-> B

Loading

Ready 40506632295 has all four jobs successful; canonical policy 40506770303 is successful. These green software checks do not cover the cases below. Static source/caller review only; no PR code, profiler, real model or hardware experiment executed. The correction to the original top-level UnicodeDecodeError claim is reasonable, but the new recursive failure path needs protection.

[Dong1017]

[P1] Propagate an unavailable submodule identity without an uncaught KeyError. repository_identity(worktree) returns {path, available: False, error} on a Git/read/filename failure, so checkout['snapshot_hash'] is absent. For example, a trial creating a non-UTF-8 untracked filename inside an initialized submodule makes this raise KeyError, which the outer catch does not handle. harness/runner.py:112 calls snapshot in finally before terminal evaluation/status/report writes, so those artifacts are lost. Check child availability first and raise a contextual caught ValueError (making the parent unavailable), rather than indexing success-only fields or treating a failed child as verified. Add a full-run regression for a failing submodule snapshot.

[Dong1017]

Fixed in 4060e80.

_submodule_facts now checks checkout["available"] first and raises ValueError(f"Submodule {path} identity unavailable: {checkout["error"]}"), which the outer except (OSError, ValueError, ...) in repository_identity catches, so the parent snapshot records available: False with a message naming the submodule. No partial identity is written: the branch returns only when available is true.

Reproduced first (a non-UTF-8 untracked name injected into the submodule falls out of repository_identity as [available, error, path]): base raises KeyError: snapshot_hash out of harness/runner.py::execute_checked and the finally block never writes evaluation.json or report.html.

Regression tests, both failing on 1e76ef6:

• test_unreadable_submodule_identity_names_the_path_instead_of_a_key_error — repository_identity returns unavailable with Submodule sub identity unavailable: Untracked path is not valid UTF-8: ..., not KeyError.
• test_unreadable_submodule_identity_keeps_run_artifacts — the same failure inside run_experiment leaves evaluation.json, report.html and status.json (state: completed), verdict INCONCLUSIVE, source_integrity.verified: false.

[Dong1017]

[P2] Hash directory symlink entries in the nested-repository walk. os.walk places symlinks to directories in subdirectories, but with its default followlinks=False does not visit them; only filenames are added here. In an untracked vendored clone reported as nested/, retargeting nested/package from dependency A to B therefore changes executable source selection without changing this available snapshot. Record directory symlinks using the existing _file_digest link-target handling, without following them, and add a retargeting regression.

[Dong1017]

Fixed in 4060e80.

_directory_digest now inspects subdirectories before reassigning the walk list: every non-.git entry that Path.is_symlink() reports is appended as a path\0_hash entry using the existing _file_digest (which hashes os.readlink output) and is removed from the descend list, so it is recorded but never followed.

Verified behaviour difference on this host at logic level, because symlink creation here fails with WinError 1314 (unprivileged Windows): with a simulated os.walk(followlinks=False) yield, base returns the same digest after retargeting nested/package A -> B; the fixed code returns different digests. The committed test test_directory_symlink_retarget_changes_directory_digest exercises this with a real symlink and calls self.skipTest(f"Symlinks unavailable: {exc}"), matching the existing pattern in tests/test_evidence_inventory.py and tests/test_skill_library_readers.py. It skips on this host and would run on POSIX CI.

Confirmed note from your review: NTFS junctions are not symlinks and is_symlink() is false for them, so os.walk still descends into them and a junction retarget is already caught by the content hashes.

[Dong1017]

[P2] Verify device records, not arbitrary tokens anywhere in tool stdout. Requested ID 3 passes if the only actual NPU is 0 but a temperature/memory/other field contains token 3; the requested model can independently match a different device's row. Also, the CLI defaults to no IDs/model, so even empty stdout with exit 0 reaches available / selection_verified=True. These still admit unsupported device evidence through the new CLI gate, the core #178 defect. Parse supported backend device records, bind IDs and model to the same selected records, and leave empty/unrecognized output non-executable. Add negative cases for an unrelated numeric field, model on another device, and empty default-selection output.

[Dong1017]

Fixed in c3940d2.

parsed_tool_facts no longer tokenizes stdout. It takes a per-tool record reader and qualifies a selection only against parsed records:

• npu-smi: one record per pipe-framed NPU row whose first cell is <id> <name> (e.g. | 0 910B3 |). The chip row repeats the NPU ID with the chip index (| 0 0 | 0000:C1:00.0 ...), so only two-token rows with an alphabetic name are accepted. Unframed or unrecognized text yields no records.
• rocminfo: one record per Agent N block, accepted only when it declares Device Type: GPU. A host CPU agent also reports a Uuid and a Marketing Name, so keying on those alone would have qualified a CPU-only host — that is covered by a test.

Then: no records -> unavailable with No device records parsed from {tool} output (so empty stdout, exit 0, no requested IDs is not available and selection_verified is false); requested IDs must appear as record IDs; the model is checked only against the records carrying the requested IDs, so an ID and a model on different rows do not combine. Model comparison normalizes whitespace, -/_ and a leading Ascend, so --device-model Ascend910B matches a reported 910B3.

cuda.py and tool_facts are unchanged. cli.py already required both status == "available" and selection_verified in run and doctor, so it needed no edit.

Tests (each fails on 1e76ef6): test_ascend_stray_number_is_not_a_device_id (ID 3 only in a temperature cell), test_ascend_empty_output_never_qualifies_a_device, test_ascend_unrequested_ids_still_require_records, test_ascend_id_and_model_must_match_the_same_record, test_rocm_cpu_agent_is_not_an_accelerator, test_rocm_empty_output_never_qualifies_a_device.

Limitation to flag: I could not obtain a real npu-smi info table from any source in this workspace and there is no Ascend host available to me, so the row shape above comes from the documented framed layout, not from captured output. It is deliberately conservative: a different npu-smi version whose rows do not fit that shape produces zero records and therefore unavailable (doctor exits 1, gated with a reason) rather than a false available. If you can paste real output from the target host, I will pin the parser to it and add the sample as a fixture.

[Dong1017]

Incremental review of c3940d2 (previously inspected 1e76ef6; PR base 5c6a8f6). The three prior failure mechanisms are corrected: unavailable child identity follows the caught failure path; directory-link targets enter the snapshot; device qualification now uses records rather than arbitrary token matches. One new P2 remains below, so not LGTM yet. Ready CI 40632971132 passed all four jobs. Verified the ordering defect with the exact fetched module in a credential-free isolated process and temporary Git repositories; no full local suite or accelerator experiment was run. Real Ascend layout coverage and author-local Windows symlink skips remain verification limits, not new feature requirements.

[Dong1017]

[P2] Sort directory symlinks before hashing. links retains filesystem enumeration order, unlike the sorted directories and regular files. In an untracked nested Git repository with a -> target and b -> target, unlinking and recreating a with the identical target changes the snapshot digest despite identical final paths, bytes, link targets and HEAD. Reproduced through repository_identity; both identities remain available. This can falsely report source drift and make a comparison INCONCLUSIVE. Iterate over sorted(links) (or canonicalize all entries before hashing) and add an unchanged-target recreation/order regression alongside the retargeting test.

[Dong1017]

Fixed in b4593e61.

_directory_digest now sorts links before hashing, so the digest depends only on the path/link-target set rather than on filesystem enumeration order.

New test test_identical_symlink_recreation_keeps_the_directory_digest creates a and b both pointing at shared/, unlinks a, recreates it with the identical target, and asserts the digest is unchanged. It fails on c3940d2f (assertNotEqual-style drift: the digest changes) and passes now.

Note on the test environment: symlink creation raises WinError 1314 on this unprivileged Windows host, so this test and the existing retargeting test both skip here and run on POSIX CI. That is the same limitation I recorded for the retargeting test — the ordering bug itself was confirmed by reading os.walk semantics (subdirectories is unsorted) rather than by a green local run.

[Dong1017]

LGTM for the incremental sorting fix, reviewed c3940d2 → b4593e6. The previously reported directory-link ordering defect is fixed. Credential-free POSIX tests using exact fetched modules reproduce drift on the old version and stable identity on this head after identical-link recreation and reversed enumeration; genuine retargeting still changes identity. Inspected adjacent tests and snapshot/runner consumers. Ready CI 40645329037 passed all four jobs. No full local suite or accelerator experiments; Windows symlink skips and real Ascend-layout coverage remain verification limits, not newly imposed requirements.

Latest canonical policy: pending (https://github.com/InferMatrixLab/InferRSI/actions/runs/40647303671). Code-review conclusion is scoped; merge still requires current-head admission to finish successfully.
This review is not automatic merge, issue closure or campaign acceptance.

[Dong1017]

Integration-status update at unchanged head b4593e618cf24cdc036a194e2af2b518a91cd023, PR API base 5c6a8f6af7ecd8fa71cedd31889820eb1546aa50, against observed main 0b19c5c3b649ebac939ffecae5583d3a22f598e7.

GitHub now reports mergeable=false. The existing scoped review remains evidence for its reviewed content; it does not clear integration with this newer main. The current-head Ready jobs passed and the queried per-head admission status is successful, but those results do not establish a successfully tested new combination.

Next owner/action: the existing PR/governing-Issue owner should reconcile current main. Reconcile this branch with current main while preserving the source/device-identity fixes and both sides' regressions, then validate the resulting head. This is an integration prerequisite; it does not reopen already corrected source findings or add a model/hardware-validation requirement. No conflict-file attribution is made without a merge reproduction.

Verification: remote refs, existing discussion/review and pinned CI inspection only; no code execution, branch change, workflow rerun or merge. This is not a new LGTM or Issue acceptance.