Repository navigation
feat(acq-kits): install OpenCode v2 via opencode kit - #466
Conversation
6c81f18 to
6423ca6
Compare
mogul
changed the title
|
Reviewed at 6423ca6. This migrates the OpenCode acq kit from the 1.18.34 GitHub release binary to pinned 2.0.22 npm platform tarballs, adds AVX2/baseline variant selection, and expands the kit's offline verifier and docs. It also replaces the repository's markdownlint implementation and deletes the newest generated changelog section, neither of which is part of the stated intent. Intent: The npm tarball selection, pre-extraction hash check, archive-member checks, and offline tests match the stated migration. But the upgrade failure path can retain the old v1 agent instead of leaving it absent, and three unrelated repository changes regress release history, markdownlint's single-source file selection, and a CI guard. Findings (4)
Validation gaps: The repository's own validators did not run for either the author or this review — Below the bar: The same-version idempotency path trusts the installed binary's self-reported Verdict: needs changes Three of the four blocking findings (2, 3, 4) are in files unrelated to installing OpenCode and would be easier to review, and to revert, as a separate change. Assisted-by: OpenCode [gpt-5.6-sol-latest-guardrails-defaultv2, claude-opus-5] team-code-review |
|
Addressed the review feedback in Changes made:
On the markdownlint issue: I agree it should not be fixed independently in each acq-kit PR. The aligned fix is the shared markdownlint dependency/config path from Verification: I also ran two adversarial review passes after the fix. The first caught the early-exit stale-binary gap; after moving cleanup earlier and adding the invalid-version regression, the second reported no blocking findings. Residual known risk remains unchanged from the original review: same-version idempotency trusts the installed binary's Still blocked locally: live |
Update the opencode acq kit to install OpenCode v2 and switch markdownlint to an auditable CLI path.
basiliskus
left a comment
basiliskus
left a comment
There was a problem hiding this comment.
Re-reviewed at cdc27b59. This head is a pure rebase of e62224d9 onto dd22daff: the reviewed base-to-head diff is byte-identical, so the verdict and exit-path audit carry over unchanged. My earlier comment was pinned to 6423ca69 and is now stale — all 4 findings are resolved.
Finding 1 (stale binary on a failed upgrade) — resolved. This was the one that needed real checking, since presence of a removal isn't the same as correct placement, and your own note said the first attempt landed too late for some early exits.
I enumerated every non-success termination in opencode-install.sh: 20 outcomes. 19 exit 0, and all 19 remove a pre-existing wrong-version binary before terminating — the three validation exits (:98, :110, :117) each call removal explicitly, and everything downstream is covered by the unconditional removal at :139. The post-install fallthrough at :305-306 removes a non-runnable replacement as well.
The only outcome that preserves a stale binary is "existing destination cannot be removed" (:85), and that exits 1. That's the right shape: the original concern was a false-ready at the wrong version, which needs exit 0 to satisfy the agent: readiness gate. An exit-nonzero path that leaves the old binary in place can't produce that.
The new tests are causal, which is what makes this convincing. I ran the verifier myself rather than relying on the output in your comment: 19 passed, 0 failed at this head. I then re-ran the same verifier with only the installer swapped back to 6423ca69 and got 16 passed, 3 failed — stale v1 survived both the download failure and the invalid-version validation, and the removal-before-download ordering assertion failed. The new assertions fail against pre-fix code for the intended reason.
One coverage note, not a blocker: the two scenarios pin one early branch and one downstream branch rather than the whole degraded class. A table-driven stale-destination test across each injectable failure class would catch a future exit added above the removal point. Worth considering, not worth holding the PR for.
Findings 2, 3 and 4 — resolved. CHANGELOG.md is out of the diff, so the published 1.11.0 section is intact. .pre-commit-config.yaml, .github/workflows/ci.yml and .github/linters/ are all absent, which restores the single-source markdownlint path from GSA-TTS/agentic-coding-patterns#451 and with it the real markdownlint runner and the original non-zero file-count assertion. I agree with your reasoning on keeping that fix central.
The earlier Markdown Lint failure was inherited from main, and it was partly a consequence of my own review feedback asking you to drop the local shim. You then fixed it centrally in GSA-TTS/agentic-coding-patterns#471 by replacing markdownlint-cli2 with [email protected] plus the js-yaml override. That is the better outcome: this PR is now 7/7 green, the repository keeps one markdownlint path, and dropping the hand-rolled walker avoids both defeating the CI file-count assertion and introducing subtle behavior differences such as the symlink-handling case you caught.
What I did not verify, stated plainly. My assessment is on the static merits only. The live install path has still never been executed by either review of this PR:
RUN_ACQ=1verification: BLOCKED,acqunavailable. No sandbox, no real npm download, no egress, no binary compatibility, no end-to-end readiness wiring.make validate(Error 127, nopython),scripts/validate_repo.pyandvalidate-kits.py --strict(bothModuleNotFoundError: jsonschema): all BLOCKED in my environment. CI's Pattern Validation is green, but I didn't read its raw logs.- The three pinned npm SHA-256 values are unchanged from
6423ca69and were never independently re-derived, here or in the first pass.spec.yaml's own comment asks for that on a bump. If anyone has re-derived them from the real tarballs, that's worth saying on the PR. - Residual risk you already acknowledged, unchanged: the same-version idempotency path trusts the installed binary's
--versionrather than re-hashing it.
So from my side the blocking findings are closed and I have no new ones. The gap that remains is live execution, not code.
Thanks for the thorough fix and for the adversarial pass that caught the placement problem before I saw it.
Remove a non-pinned existing opencode binary before degraded replacement paths so a failed OpenCode v2 install cannot leave a stale v1 binary satisfying readiness checks. Also drop unrelated markdownlint and changelog deltas from this PR branch; those should be handled by the shared markdownlint dependency fix instead. Co-authored-by: OpenCode Agent <[email protected]>
|
I fixed a couple of cpu detection differences that only appeared on my MacOS host through live verification. They're minor enough that I'm not going to ask for a re-review. Merging! |
…im comment narration per mogul's review Two fixes, both from mogul's CHANGES_REQUESTED review: 1. Rebase onto main pulled in the opencode kit (#466), which landed with a verify script in neither CONVERTED nor UNCONVERTED, failing test_every_kit_is_classified exactly as the test is designed to catch (reproduced locally before fixing: `AssertionError: verify script(s) in no classification set: ['opencode']`). opencode's verify script is on the old two-state verdict, so it goes in UNCONVERTED alongside the other not-yet-migrated kits. 2. Trimmed six comment/docstring blocks that narrated the OLD two-state verdict's history ("used to end with...", "the old reporters had no...", the oci-engine near-miss anecdote) instead of describing the current code. Per review: this kind of narration belongs in the commit message, not in the code, where it has no reader value and persists as noise. Trimmed in: - integrations/isolation/acq-kits/git-ssh-sign/scripts/verify - integrations/isolation/acq-kits/usai-provider/scripts/verify - integrations/isolation/acq-kits/README.md - integrations/isolation/acq-kits/verify-report.sh (module header) - scripts/tests/test_verify_contract.py (module docstring + 2 inline comments + 1 docstring) Comment-only / allowlist-entry changes: no test logic, no shell logic, no shared-library behavior changed. Per the review's own standing offer ("if you respond with improvements that only land in comments... feel free to check the box that skips the require-review gate"), this should qualify to merge without a second review round-trip. Verification: - PYTHONPATH=scripts python3 -m pytest scripts/tests/: 420 passed (was failing 1/420 before the opencode classification fix, reproduced first). - python3 scripts/validate_repo.py: all validators pass. Co-authored-by: OpenCode Agent <[email protected]>
Summary
Update the
opencodeacq kit to install the current stable OpenCode 2-series CLI,2.0.22, from pinned npm platform tarballs instead of the old GitHub release-binary path.The kit now fetches and verifies:
@opencode/[email protected]@opencode/[email protected]@opencode/[email protected]Content Type
Changes
2.0.22and update SHA-256 values for supported Linux platform tarballs.registry.npmjs.org.Safety Checklist
prohibited_contentnot applicable; this is an integration kit update, not a pattern fileQuality Checklist
Verification Transcript
Blocked/partial checks:
Security Impact
This changes the agent-harness installation source from GitHub release tarballs to official npm platform tarballs. It preserves SHA-256 verification before extraction, rejects unexpected archive layouts, removes a non-runnable installed binary, and does not run npm lifecycle scripts.
Rollback
Revert commit
8029510to restore the previous OpenCode 1.18.34 GitHub release-tarball install path.AI Disclosure
This PR was prepared with OpenCode AI assistance. Human review is required before merge.