Repository navigation
feat(spec): let a provider declare the inputs a compile read - #106
Conversation
LPP 1.7 adds the independently negotiated compileInputs capability. An entry-based lpp/compile result then lists every file and directory listing the compile read, as sorted absolute file URIs, outside the opaque artifact. The reference mock provider implements it, fixtures 69-70 cover the advertised and unadvertised sessions, and the runner rejects the field without the negotiated capability. ADR 0007 records the field shape and the rejected alternatives. Fixes #105
|
Review verdict: CHANGES REQUESTED Reviewed head b3472dd. Gates pass locally: fmt, clippy 1. The normative guarantee excludes entries of listed directories, so a client cannot ignore edits to files the compile never read
Under this wording, every entry of a listed directory has no guarantee for any kind of change, edits included. Fixture 69 shows the problem. The mock lists The spec also does not define what a client must compare for a listed directory. The mock's result depends on more than the entry names. Required correction: state the guarantee as a function of the declared inputs. For example: "The result depends only on the content of each listed path that is a file, the existence of each listed path, and, for each listed directory, the names and kinds (file or directory) of its entries. A change that leaves all of these unchanged cannot change the result." Make the ADR consequence match this text. Rule: issue #105 acceptance criterion "the spec defines the capability and field" and its desired behavior ("an edit to any file outside the set cannot change the result"). No WrightKit policy rule covers this. 2. The mock declares canonical paths, so retargeting a symlink changes the result without changing any declared input
Required correction: in §10.2, require that every path the provider traversed, including each symlink it followed, is listed. In the mock, include the pre-canonical path next to the canonical one. A fixture is not needed if the runner cannot create symlinks portably. The spec text and the mock must agree. Rule: no WrightKit policy rule covers this. It breaks the PR's own normative guarantee in the reference implementation (repo 3. Method table entry for
|
State the compileInputs guarantee as a function of the listed paths: existence and kind, symlink targets, file content, and directory entry names and kinds. An edit to an unlisted file inside a listed directory no longer counts as a possible change. Require every followed symbolic link to be listed with its resolved path. The mock now declares traversed paths before resolution, and a stdio test covers a symlinked source. Restore the list punctuation in the conformance method table. Refs #105
|
Review fixes pushed in cbb41e9 (no force push). All three findings from #106 (comment) are addressed. 1. Guarantee stated as a function of the declared inputs
2. Followed symbolic links are declared
3. Method table wording
Gates at cbb41e9
Please run a follow-up review. |
|
Review verdict: CHANGES REQUESTED Follow-up review at cbb41e9. Findings 1 and 3 are fixed. Finding 2 is fixed in the spec and for a single link, but the mock still breaks the rule for links that the provider follows while it resolves a link target.
2 (still open). The mock declares only the first link and the canonical path, not every link it followed§10.2 now requires "every symbolic link the provider followed". Reproduced against the mock at cbb41e9 (
So the reference provider still lets a result change while no declared input changes. This is the guarantee from finding 2. Required correction: resolve each traversed path one component at a time and declare every link encountered, including links reached through a link's target. Alternatively, declare Rule: no WrightKit policy rule covers this. It is the PR's own §10.2 rule, and the repo |
Resolve each traversed path component by component and record every symbolic link on the way, including links in a chain and linked directory components. A stdio test covers a two-link chain and a linked directory. Refs #105
|
Round-2 fix pushed in f0159e8 (no force push). It addresses the remaining item from #106 (comment). Only the mock and its tests changed. Intermediate links are declared
Gates at f0159e8
Please run a follow-up review. |
|
Review verdict: LGTM Follow-up at f0159e8. Finding 2 is fixed. The commit changes only
|
Summary
LPP 1.7 adds the
compileInputscapability. A provider that advertises it returnscompileInputson every entry-basedlpp/compileresult: a sorted, duplicate-free array of absolutefile:URIs. The array lists every file whose content the compile read and every directory whose listing it read, including non-source files (for example compile-time scripts) and probed-but-missing paths. A change to a path that is neither listed nor an entry of a listed directory cannot change the result.The field sits outside the opaque artifact. It is gated like
sourceIdentity(LPP 1.3): request"1.7"and requirecompileInputs: true. Document-supplied results and pre-1.7 sessions never carry it.Field shape: trade-offs (also in ADR 0007)
filestableChanges
1.0–1.7.compileInputs, returns the project directory and loaded.xdlfiles.69-compile-inputs(advertised; a non-sourcenotes.txtis not an input) and70-compile-inputs-unavailable(--without compileInputs, field omitted);sessions/1.7.json.invariantsmodule checks every actual compile response, independent of the fixture. It rejects the field without the negotiated capability, on document-supplied results, or with a malformed shape, and rejects an entry result that omits it after negotiation. Unit tests cover reject and accept; a session test proves a provider fails even when the fixture expects the field.Verification
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings: clean.cargo test --workspace: all pass.lpp-conformance-runner --validate-only: 70 passed.lpp-conformance-runner --provider lpp-mock-provider: 70 passed.git diff --check: clean.Not covered: the OPY provider (wrightkit/opy-rs#659) and the Wright consumer (wrightkit/wright#1005) are separate Issues.
Fixes #105