Make sample goldens reproducible from the source tree - #605
Conversation
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review — round 1
Reviewed the actual published 13-file diff and matched every blob at cd60c58e38f08fe9bb1adb98ac53900c2affb967 against 54b231ce00e11ecf62e16cdc075bb3bac40264f6.
One actionable finding: the new CRLF test corrupts already-CRLF fixture inputs into CRCRLF and can fail on a normal Windows checkout. The inline comment at tests/test_regenerate_goldens.py:199 includes the disposable reproduction and bounded repair. This is a test-portability defect; the same probe confirms the generator handles valid CRLF correctly.
The three pre-PR findings are addressed: scans explicitly disable plugins, fixture ancestor paths are checked before generation/writes, and machine-path checks include the product Markdown and HTML representations. Git environment/configuration is isolated and restored. Normalization precedes scan-pointer rebinding, and the pointer retains scan-only control.
Independent validation:
- 79 generator/report/packet tests passed, including all 16 new generator cases on this LF checkout.
- A separate disposable CRLF probe passed with valid CRLF inputs and reproduced the failure after the test's second conversion.
- Both changed report JSON files and packet JSON have unchanged parsed content. The four-field refund summary now matches the unchanged report (3 critical, 10 high, 2 medium). Pointer changes are confined to the updated report binding, control identity and predecessor; its permissions are unchanged.
- Diff checks passed and the worktree remains clean.
No source/control artifacts were modified and no full suite or verifier was run against this worktree. This is a coding-agent COMMENT review, not human approval or release qualification.
| target = _copy(tmp_path, sample) | ||
| for path in target.rglob("*"): | ||
| if path.is_file() and "expected" not in path.relative_to(target).parts: | ||
| path.write_bytes(path.read_bytes().replace(b"\n", b"\r\n")) |
There was a problem hiding this comment.
[P2] Make the CRLF fixture conversion idempotent
The fixture may already contain CRLF after a normal Git for Windows checkout: .gitattributes disables EOL translation only for expected artifacts, not these inputs. Replacing every LF directly then turns each CRLF into CRCRLF. The generator's universal-newline read correctly interprets those as extra line breaks, so the manifest digest and current-control golden change and this test fails even though generation from valid CRLF works. On a disposable conductor copy at this head, valid CRLF inputs matched the committed artifacts; applying this exact conversion again produced CRCRLF and a different current-control.json. Normalize the source text to LF before constructing CRLF bytes, and verify the test setup works from both LF and already-CRLF inputs.
Addressed review round 1Review 5149681209 is addressed in Only the test changed after the reviewed |
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review — round 2
Reviewed the published head dd6d629e86bcb8ddb32b9601ef3b9a4181954bab against base 54b231ce00e11ecf62e16cdc075bb3bac40264f6, including the delta from round 1 at cd60c58e38f08fe9bb1adb98ac53900c2affb967. The only changed file since that review is tests/test_regenerate_goldens.py; its published blob matches the worktree. The other 12 published blobs are unchanged.
Disposition: the round-1 P2 finding is addressed; no remaining actionable in-scope findings.
The test now normalizes text through universal-newline reading before producing CRLF bytes. Its one/two-conversion parameterization covers both LF and already-CRLF starting inputs without creating CRCRLF. This repair changes no generator behavior, golden artifact or control permission. The earlier containment, plugin isolation, escaped-path checks and final-byte pointer binding remain as reviewed.
Independent validation:
- All 17 generator tests passed, including both conversion cases across all seven recipes and the actual 24-artifact CLI check.
- A separate disposable conductor probe applied the fixed conversion twice. Neither pass produced CRCRLF; all five generated artifacts matched the committed bytes after each pass.
- The author address record links the finding and this exact fix. Diff checks passed and the worktree remains clean.
This is a coding-agent COMMENT review, not human approval or release qualification. No source/control artifacts were modified, and no full suite or verifier was run against the implementation worktree.
Addressed review round 2Review 5149767800 confirms the final head This completes two GitHub coding-agent review/address rounds. Final-head CI and aggregate coverage must still complete before merge; the local 9,291-pass full suite and coding-agent review are not release qualification. Current control was refreshed before this update. |
1 participant
After a report or contract change, contributors could only rebuild sample goldens with separate manual recipes.
scripts/regenerate_goldens.pynow owns all 24 committed artifacts across seven samples, with all/sample selection and a read-only--checkthat names drift. The normal CI suite invokes that exact entry point.The recipe uses real built-in-only scans in disposable Git copies, preserves committed versus cold-manifest states, pins packet time, normalizes LF inputs and known output paths, and rebinds the Conductor scan pointer after its final bytes. It isolates inherited Git/summary settings, refuses symlinked fixture paths and detects raw, JSON, Markdown and HTML spellings of generating-machine paths before writing. No sample declaration or engine/gate behavior changes.
The existing goldens needed five explicit corrections: two JSON reports now use the product writer's Unicode encoding; the Conductor pointer binds those final bytes with no prior generating run as its predecessor; the packet JSON follows its writer's ordering/terminator; and the stale four-field refund summary now matches its unchanged current report (3 critical / 10 high / 2 medium, previously 2 / 14 / 2). The existing verdict/questionnaire/packet assertions remain independent of the generator.
CONTRIBUTING and schema/contract guidance link the committed recipe; both sample README recipes now use it. These are development fixtures, not beta receipts, a report 1.0 freeze or v1.0 qualification. #569 retains those obligations.
Validation: full local suite 9,291 passed, 5 skipped; 161 focused generator/report/packet tests passed. The generator's 17 cases cover same-byte regeneration across roots and CRLF inputs, repeated conversion of an already-CRLF checkout, read-only one-byte drift/missing-file controls, plugin refusal, symlink containment and temporary-path refusals. The actual CLI reports 24 artifacts with no drift; Ruff and diff checks passed. Formal GitHub review/address evidence is recorded below.
Closes #499. Refs #494, #569, #572.