[#1276 follow-up] Lint provisioning: installer digest-tuple + before-Claude tool install + setup lint_mode + review-runner isolation - #1963
Conversation
…state + lint_provision) (checkpoint)
…-end fail-closed tests (checkpoint)
…e-workflow wiring + review-runner trusted-base isolation (checkpoint)
…er + generator + drift gate + .gitignore re-include (checkpoint)
…arker; vendor-slice ships both; record-path skew support (checkpoint)
…ds) + gitignore comment reconcile (checkpoint)
…g step; ruff noqa (checkpoint)
… registration, marker regen
…w-up-lint-provisioning-installer-digest # Conflicts: # lib/test/test_python_scripts.py
…w-runner hardening, single --version exec, exit-code clarity)
…ken version match, cache-path honesty, Plan/StateResult/Readiness invariant guards, +zip/reuse/matrix tests
…r the {OS,arch,tool,version,digest,installer} key (#1388)
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…w-up-lint-provisioning-installer-digest
#1388 slice) so DEVFLOW_VENDOR install lands
|
/prflow:review |
PRFlow Review — PR #1963Status: ❌ REJECT Blueprint
Run telemetry: 3 checklist-generators + 1 deduper + 56 verifiers + 1 lite + 6 Phase-3 agents. Formal review posted: REQUEST_CHANGES (marker stamped). Read-only cloud review tier — durable telemetry persist is writable-run-only; no persist performed. Verdict: REJECT (an explicit acceptance criterion is not met, and two security-relevant behaviors are pinned only by string presence)This is a strong, carefully-engineered change — CI is green, the Python helpers follow the "unknown is not zero" fail-closed discipline rigorously, and the provisioning vector it introduces ( Issue ComplianceReviewed against issue #1388: [#1276 follow-up] Lint provisioning — criteria from the
Verification Checklist Results55 passed, 2 failed, 0 inconclusive — 1 via lite probe, 56 via agent.
Two verifier FAILs were disproved by the orchestrator against HEAD and are recorded PASS: VC-7 (vendor-slice.sh does ✅ Passed items (54 of 57) — click to expand
Code Review Findings🟠 Important / Major
🟡 Suggestion / Minor
over-grade annotation: findings #2 and #3 flagged shape 3 (advisory only — the verdict is unchanged by these annotations). Verdict Criteria applied
What clears the REJECTThe single required change is to reconcile AC8: either extend the base-ref hardening to VerdictREJECT — see above. |
|
/prflow:review |
PRFlow Review — PR #1963Status: ❌ Review failed Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32897245966 |
|
DevFlow review stall backstop — this cloud review ended with no verdict for /devflow:review |
Reception pass — round 3 disposition (review @ dbf1330, fixes in 64c4be4)Important
Suggestions
Verification: |
|
/prflow:review |
PRFlow Review — PR #1963Status: ❌ Review failed Blueprint
Findings (live)(Phase-3 findings appear here as each agent returns.) Verdict(pending) Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32923574003 |
|
DevFlow review stall backstop — this cloud review ended with no verdict for /devflow:review |
|
PRFlow review: no verdict-post receipt was found for this run.
No run-scoped verdict-post receipt was found for this run: either Phase 4.4's No review authored by this run's reviewer identity is recorded in the reviews Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker, This comment is a record of that gap. It is not a verdict, and it neither approves nor |
PRFlow Review — PR #1963Status: ❌ Review failed Review run failed: claude step failure — https://github.com/The01Geek/prflow/actions/runs/32924266453 |
|
DevFlow review stall backstop — this cloud review ended with no verdict for /devflow:review |
|
PRFlow review: no verdict-post receipt was found for this run.
No run-scoped verdict-post receipt was found for this run: either Phase 4.4's No review authored by this run's reviewer identity is recorded in the reviews Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker, This comment is a record of that gap. It is not a verdict, and it neither approves nor |
![prflow-reviewer[bot]](https://avatars.githubusercontent.com/u/223979750?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE with notes (all acceptance criteria satisfied and cross-file contracts verified against the shipped bytes; one Important test-coverage gap and several refinement suggestions, all below the merge-gating threshold)
Issue Compliance
Reviewed against issue #1388: Lint provisioning: installer digest-tuple + before-Claude tool install + setup lint_mode + review-runner isolation — criteria from the /prflow:implement workpad comment; scope unchanged. Requirement-based checklist items are included in the verification results below.
- Criteria came from the
/prflow:implementworkpad comment — this run's authoritative set, possibly narrowed from the issue. - No divergence between the workpad's criteria and the issue body's.
All twelve acceptance criteria were checked against the shipped code (read at head 64c4be4d, since the review checkout sits on the default branch) and each is satisfied:
- Digest-bound tuple published last (install.sh §4b validates the staged manifest, then
install_state.py buildpublishes.prflow/install-state.json— ordering driven end-to-end by newinstaller-wiring.sharms). ✓ - Fail-closed fixtures (first-install / backfill / missing-marker / version-skew / interrupted-publication) resolve deterministically before any tool work via
check_readiness. ✓ .gitignorere-includes!/.prflow/install-state.json(and the manifest); both files tracked at head. ✓- Closed
lint_modeinput;provision/noneaccepted, any other value::error::+exit 1. ✓ provisioninstalls run-local (nosudo), digest-before-extract and whole-token version verified, cache re-verified. ✓noneshort-circuits before any manifest read. ✓devflow.yml/devflow-implement.yml/devflow-runner.ymlpassnone/provision/none(pinned by count). ✓- Review-runner isolation:
hardensetupmaterializes the wholesetup-project-envdir from trustedBASE_REFFETCH_HEAD, prunes PR-added files, fails closed, and precedes theuses:step; paired withlint_mode: none. ✓ - Platform matrix declared;
unsupported-lint-platformfor the one unsupported tuple. ✓ - Six integrity arms each fail closed naming the tool. ✓
- CI
lint-manifestjob validates+exercises the manifest undercontents: read, no App token. ✓ - Uniquely-named changeset with
bump: patchfrontmatter added. ✓
Verification Checklist Results
59 verification items (deduped from 3 generator batches) — 0 failed, 0 inconclusive. The cross-file contracts (helper CLI signatures, exit-code conventions, component-name sets, the validate_state fail-closed matrix, check_readiness fixtures, the MISSING_FILE_REASON equality distinction, the URL-metachar safety invariant, the .gitignore/vendor-slice shipping, run.sh pin counts, and each acceptance criterion) were confirmed against the shipped bytes by the Phase-3 agents and by direct git show probes at head, and the green CI (all five suite shards, lib + python tests, lint, and the lint-manifest validate+exercise job) corroborates the digest-tuple, drift-gate, and manifest-matrix items.
Engine ground truth for reviewed commit 64c4be4d: lib + python tests: success, lint (shellcheck + actionlint + ruff): success, lint-manifest: success, and all five shards success.
Code Review Findings
🟠 Important / Major
- Artifact-URL and pinned-digest correctness is exercised against upstream for only the
linux/x86_64tuple (the CIlint-manifestjob downloads and digest-verifies that one); the other 8 tuples'os/arch/target-triple mappings (_SHELLCHECK_OS/_SHELLCHECK_ARCH/_RUFF_TARGET) and their pinned digests carry no string-equality or real-download assertion. A transcription error in one of those mappings or digests produces a plausible-but-wrong URL (404) or a digest mismatch that no test catches — and because a mismatch/404 on a declared tuple is a hard_die(exit 1) in an un-continue-on-errorstep, it aborts the whole implement job at setup on that platform (a hard abort, not theunsupported-lint-platformwarn-and-continue degrade) — precisely the paid-turn failure this feature exists to remove. A CI matrix that resolves+downloads each declared artifact (or per-tuple URL-string assertions) would catch it before merge. (raised by 2/6 agents — Important frompr-test-analyzer, Suggestion from the final-pass reviewer.) Below the defaultcriticalthreshold, so it does not block; worth addressing.
🟡 Suggestion / Minor
provision-lint-tools.shhard-aborts (_die, exit 1) on a missing extraction primitive (unzip/tar) for a supported tuple rather than degrading — in tension with the graceful-degradation convention this PR adds to CLAUDE.md. A deliberate, tested choice (integrity failure vs. unsupported-platform degrade), but a Windows consumer lackingunzipwould see a hard implement-run abort; worth a conscious confirmation. (code-reviewer, sub-threshold confidence)install.sh§4b silently skips the marker-publish block (nologline) when its source preconditions are absent, unlike every other §4b arm; the fail-closed consequence is correct but undiagnosed for the operator. (silent-failure-hunter)check_readinessdigest-checks each recorded component but does not assert the recorded component set is complete — a trust-only defense-in-depth gap, not a live fail-open (the marker is trusted committed/base-ref output). (silent-failure-hunter)StateResult.statestores the raw parsed dict directly, so the frozen wrapper's immutability is shallow — a caller could mutatesr.state[...]and invalidate the validated guarantee. Advisory; current callers don't mutate. (type-design-analyzer)Planlacks the.establishedboolean discriminator its siblings (StateResult.established,Readiness.ready) expose, forcing stringly-typedstatus == "established"compares at call sites. (type-design-analyzer)Plan.__init__mandates completeness only for_RESOLVED_FIELDS; the identity fieldstool/os/arch(read by thecache-keypath) may beNoneon anestablishedplan. Latent —build_planalways supplies them. (type-design-analyzer)Plan's__setattr__/__init__comments say "defeat the XOR" thoughPlanis a three-way discriminated union, not an XOR — the comment misdescribes the invariant it guards. (type-design-analyzer)Plan.__init__silently ignores unknown keyword arguments (**kw), so a typo'd kwarg surfaces as the less-direct "missing resolved fields" error; its sibling types use explicit named parameters. (type-design-analyzer)- The
devflow-runner.ymlhardensetuprationale comment block (~11 lines) exceeds the three-line prevention-comment cap and carries mechanism derivation duplicated indocs/internal/cloud-setup.md; the load-bearing "fail closed / no PR-head fallback" prohibition is the keeper. Demotable (undecidable-load-bearing carve-out plausibly applies). (comment-analyzer) - Test refinements: the version-skew "either direction" arm is exercised by a single mutation; the unwritable-target arm no-ops (false pass) under root; the full platform matrix and real downloads run only in CI, not the offline suite. (
pr-test-analyzer)
over-grade annotation: no finding flagged.
truthfulness sweep: no finding promoted — all six agents verified the doc/comment/release-note claims (the review-runner isolation claim, the "no shell metacharacter reaches the URL" invariant, the CLAUDE.md convention addition) against the shipped code and found no documented falsehoods.
intra-diff contradiction scan: no contradiction found.
Prompt-surface edit routing gate: the diff touches CLAUDE.md but no writing-skills trigger glob (skills/*/SKILL.md, skills/*/phases|references/*.md, .prflow/prompt-extensions/*.md) — the gate draws no finding this run.
Notes
- No self-contradicting-diff / documented-falsehood findings; no Phase 0.6 STALE findings (the stale-prose lint reported only UNRESOLVABLE/coverage-drop informational rows, expected because the review checkout resolves
--rev HEADon the default branch where the PR's new files do not yet exist). - This is an exceptionally well-tested, security-conscious change. The type-design (both-direction invariants + frozen
__setattr__), the fail-closed provisioning arms (each driven end-to-end), and the read-only review-runner isolation are all correct. The single Important item is a coverage gap, not a demonstrated defect.
PRFlow Review — PR #1963Status: ❌ Review failed Review run failed: claude step success but the run wrote no verdict (engine reported no error) — https://github.com/The01Geek/prflow/actions/runs/32924350684 |
|
/prflow:review |
PRFlow Review — PR #1963Status: ❌ Review failed Review run failed: claude step cancelled — https://github.com/The01Geek/prflow/actions/runs/32925877001 |
|
PRFlow review: no verdict-post receipt was found for this run.
No run-scoped verdict-post receipt was found for this run: either Phase 4.4's No review authored by this run's reviewer identity is recorded in the reviews Any verdict text this run published OUTSIDE the emitter carries no producer-emitted verdict marker, This comment is a record of that gap. It is not a verdict, and it neither approves nor |
Critical — install.sh section 4b published a marker no consumer could satisfy. The marker digested SOURCE bytes for the four copy-loop components while recording consumer runtime paths, but install_managed PRESERVES a locally modified artifact and the tier1_rc arm skips the workflow copy. check_readiness then returned digest-mismatch forever and provision-lint-tools.sh _die'd the whole implement job, with a re-run-the-installer remedy that reproduced the identical marker. The marker now describes the target tree; the new --digest-root carries the one real exception (the vendor-fetched readers, absent at install time). Proven end-to-end through the real installer in installer-wiring: a preserved local edit now converges to READY on re-run. Critical — devflow-runner.yml's hardensetup displaced PR-head bytes for the setup-project-env action but published no displaced_paths and was absent from displaced_join, so the reviewing agent read base-ref bytes as head on exactly the file a PR editing that action is under review for. It now discloses what it displaced and the join consumes it. Also: build_state validates its own output before publishing; Plan rejects unknown keywords instead of composing a "None" cache key; TOOLS is derived from the validated manifest rather than a third hardcoded copy; the SCRIPTS_DIR fallback probes both helpers and refuses when neither candidate carries them; the prune loop's membership test uses a bash builtin instead of grep (a missing grep pruned every trusted file it had just materialized); the base-ref fetch keeps its own diagnostic; _die no longer leaks its work directory; generate-install-state.py refuses an unrecognized argument instead of writing; 4b's outer guard says why it refused; __delattr__ is guarded on the immutable result types. Tests: the two Criticals, the GITHUB_PATH export, tar.xz extraction (the archive type every real shellcheck artifact uses), the drift gate's RED direction, and a readiness — not merely structural — positive control against the consumer tree. The COMPONENTS tuple's two transcriptions are now reconciled both ways round. Replaced a wording-only pin over log prose with a structural one; guarded a permission fixture on uid; trimmed the diff's over-cap comment blocks. Pushed back: the "hand-edited digest" Critical was refuted upstream — the new pins are correct and main's shellcheck pin was wrong. The missing-unzip/tar fail-closed arm is AC10's stated behavior, not a defect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new hardensetup disclosure published a second `displaced_paths<<` heredoc, which broke #504's uniqueness pin on harden_hooks' own output. Re-wording that pin is not available here — the #810 classifier refuses a changed literal, and re-adjudicating it cannot share a branch with the change that needs it. Give the new producer its own output key instead (`displaced_setup_paths`): the two producers disclose different path sets, so distinct names are right on their own merits, and harden_hooks' literal stays unique and unedited. The new #1963 assertions also added label 1963 to installer-wiring, which test_coverage_map_guard.py pins as an exact set — that one pinned set is what turned #591 and, through it, #1055 red on three shards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The three new #1963 readiness assertions moved the module's count, and the floor is an exact-policy coupled pair — the registry literal and run.sh's must move in the same commit or python-pool goes red on the mismatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/external/release-notes.md
|
/prflow:review |
PRFlow Review — PR #1963Status: ✅ APPROVE with notes Blueprint
Verdict: APPROVE with notes (correct, thoroughly-tested lint-provisioning change; CI green; only sub-threshold suggestions and one non-blocking test-coverage note)Issue ComplianceReviewed against issue #1388: [#1276 follow-up] Lint provisioning — criteria from the Verification Checklist Results61 passed, 0 failed, 0 inconclusive — 4 via lite probe, 57 via agent. Three items passed with a wording-only claim inaccuracy (the shipped code implements the targeted property correctly; only the checklist-generator's incidental locators were off):
Note: an initial verification pass mis-graded VC-27/VC-51/VC-52 as FAIL; a focused re-verification confirmed each is a clean PASS (the fail-closed fixtures do live in installer-wiring.sh; run.sh pins the provision_env gate count as 2 and the BASE_REF/git-fetch counts as 5/5, all matching the shipped workflow — consistent with the green ✅ Passed items (58 of 61) — click to expandAll checklist items resolved PASS. Highlights verified against PR-head content (
Code Review Findings🟠 Important / Major
🟡 Suggestion / Minor
ℹ️ Notes
Verdict Basis
Run telemetryPer-phase call/token/wall-clock figures were not established this run → Subagent effectiveness traceIteration 1
|
1 participant
Summary
.prflow/lint-manifest.jsonbefore the Claude action starts, so implement runs no longer spend paid turns rediscovering and installing lint tools..prflow/install-state.jsononly after the staged set validates — a run refuses to provision when the marker is absent, a digest disagrees, the manifest is missing, or a component is out of step with its declared peer.setup-project-envaction body from trusted base-ref bytes and runs withlint_mode: none, so a PR editing that action cannot execute its edit in the review job.Changes
Lint-provisioning core (Python):
scripts/lint_provision.pyresolves the (os, arch, tool) tuple to a pinned artifact + trusted URL and computes the AC5 cache key;scripts/install_state.pybuilds/parses/validates the marker and gates provisioning through a fail-closedcheck_readiness;lib/generate-install-state.pygenerates the tracked marker and backs the drift--check.Composite action:
.github/actions/setup-project-env/action.ymlgains a closedlint_modeinput (provision|none); an unknown value is refused.provisioninstalls the manifest's ShellCheck/Ruff run-local (nosudo), verifying artifact digest and executable version before readiness and re-verifying cache restores under the key{OS, arch, tool, version, digest, installer version}.nonedoes no lint-tool work and validates no manifest. The end-to-end installer is.github/actions/setup-project-env/provision-lint-tools.sh.Workflow wiring:
devflow.yml,devflow-implement.yml, anddevflow-runner.ymlpasslint_modenone,provision, andnonerespectively.devflow-runner.ymladds ahardensetupstep that fetches the trustedBASE_REF, materializes every.github/actions/setup-project-envfile fromFETCH_HEAD, prunes PR-added files, and runs before theuses:step.Installer + shipping:
install.shships the manifest and publishes the marker only after validating the staged set.vendor-slice.shcopies both.prflow/lint-manifest.jsonand.prflow/install-state.jsonto consumers..gitignorere-includes the marker beside the manifest so both ship.CI:
ci.ymladds a candidate-manifest job undercontents: read(no repository write credentials) that validates and exercises the manifest matrix plus marker readiness/drift.Resolves
Resolves #1388
Test Plan
lib/test/test_python_scripts.py[#1276 follow-up] Lint provisioning: installer digest-tuple + before-Claude tool install + setup lint_mode + review-runner isolation #1388 blocks pass:build_plantuple/artifact resolution,cache_keyshape, theinstall_state.validate_statesix-shape fail-closed matrix,check_readiness(first-install / backfill / version-skew both directions / component-removed / manifest-missing), and theprovision-lint-tools.shend-to-end fail-closed arms.run.shcross-file pins pass: the composite-actionlint_modecontract, the three-caller wiring counts, the review-runner isolation (hardening precedes use, whole-dir base-ref materialization), and the ship pins for the manifest + marker.lib/generate-install-state.py --checkpasses against the committed marker.Visual Changes
N/A
Breaking Changes
None
prompt-surface growth: no tracked
*.mdunderskills/,agents/, or.prflow/prompt-extensions/changed between24cd458853ad38f818b696d4ff2b421b50d545b2and949de2d15b8803a8785f17964bec202636028ef2— no table rendered.Generated via /prflow:implement (v2.34.22, claude-opus-4-8, low)