Repository navigation
fix(verify-libghostty-vt-node): refuse supplied libghostty-vt, check the addon hash, stop on failed writes - #52
Conversation
…anged doctor.sh now hashes the addon that node-gyp-build picks and fails when it differs from addonSha256 in instance.json, so a rebuilt, swapped, or corrupted addon no longer passes as the launched one. Change-Id: Ic83ef034aa939858a9ae72cfe2ebf27bb102a001 Signed-off-by: Thomas Kosiewski <tk@coder.com>
drive.sh exits 2 before node runs when it cannot write the journey file, its evidence copy, or command.txt. Before, node could run a stale or partial journey and report its exit code as the verdict. Change-Id: Id057e453363d5b5fbe001323f0333ce7c4f4c987 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ch.sh launch.sh now builds and links only vendor/libghostty-vt and exits 2 when LIBGHOSTTY_VT_PREFIX, LIBGHOSTTY_VT_INCLUDE_DIR, LIBGHOSTTY_VT_STATIC_LIB, or LIBGHOSTTY_VT_LIB is set. It also takes no RUN_ROOT argument: it always makes one with mktemp under TMPDIR, which removes the custom-root ownership checks. Refs #27 Change-Id: If7fb8dd7beeedf47b80f3846512a0e86ec3baa52 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c2d4853a
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: d6c2d4853a
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…can change launch.sh makes RUN_ROOT under TMPDIR, and the compiler writes temp files there. It now exits 2 when TMPDIR or a directory above it is owned by another account (root excepted) or writable by group or others without the sticky bit, and it exports the checked physical path to the build. Change-Id: I87777d4111860cd4da47ce3b7b049cab3c4b7c27 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… compgen launch.sh checked the variables with compgen, which a bash built without programmable completion lacks, and '|| true' hid that failure. It now tests each variable name directly. Change-Id: Ib6bdafdfd826ab72e46a072db98c07d6f3a4c707 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…unch advice launch.sh takes no RUN_ROOT argument, so the STALE and checkout-changed messages now say to run launch.sh again. Change-Id: I4f2063338fc2fefbd7a979a6ee9633e07c35ad66 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…pt-hardening Change-Id: I277b7984cb0ecc84219a8548211a22253e73163b Signed-off-by: Thomas Kosiewski <tk@coder.com>
## Why Ship the fixes merged since `v0.1.0-beta.3` to npm, so downstream projects such as agent-tty can update. The consumer-visible change is #53: `getNativeInfo()` now reports the Ghostty commit the addon was built from (`ghosttyCommit`), and `ghosttyVersion` reads `0.1.0-dev+<commit>`. ## What changed - `package.json` and `package-lock.json` go from `0.1.0-beta.3` to `0.1.0-beta.4`. The branch name matches the version, so `publish.yml` creates `v0.1.0-beta.4` after merge. - The `release-changelog` workflow adds the CHANGELOG entry to this branch. ## Scope - Included since beta.3: #53 (Ghostty commit in `getNativeInfo()`), #51 (7-day `min-release-age` for this repo's own npm installs, `devEngines` npm >= 11.10), #52 (verify skill scripts, not in the npm tarball), #50 (skill sync, not in the tarball). ## Blast Radius The merge publishes `0.1.0-beta.4` to npm under the `beta` dist-tag. `latest` stays on `0.1.0-beta.0`. The tarball's `package.json` now carries `devEngines`. npm checks it only for the project it runs in, and the #51 verifier installed a packed tarball with npm 9.9.4, 10.8.2, 10.9.8 and 11.20.0 (all exit 0). `.npmrc` is not in the tarball. ## Verification - `prepare-release-changelog.mjs` prints `should_generate=true` and `version-changed`. `resolve-npm-publish-tag.mjs` prints `beta`. - Local gate on this branch with Node 26.12.1: results posted as a PR comment. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Why
The user goal is that every open issue on this repo is fixed and landed on main. Issue #27 lists three gaps in the
verify-libghostty-vt-nodeskill scripts. Each one lets an agent get a verdict for a build or a journey other than the one it launched or submitted.What changed
launch.shbuilds and links onlyvendor/libghostty-vt. It exits 2 whenLIBGHOSTTY_VT_PREFIX,LIBGHOSTTY_VT_INCLUDE_DIR,LIBGHOSTTY_VT_STATIC_LIB, orLIBGHOSTTY_VT_LIBis set. It also takes noRUN_ROOTargument now: it always makes a new one withmktempunderTMPDIR, and it refuses aTMPDIRthat another account can change.doctor.shhashes the addon thatnode-gyp-buildpicks. It fails when the hash differs fromaddonSha256ininstance.json.drive.shexits 2 before node runs when it cannot write the journey file, its evidence copy, orcommand.txt.Scope
SKILL.md.features/unchanged.Tradeoffs
SKILL.mdorfeatures/uses them. Using them would mean recording a library that launch did not build and protecting an external prefix from the rebuild. Refusing them is simpler, as the issue suggests.RUN_ROOT: it needed checks on a path that the caller picks or creates, and those checks drew new findings in each review round of chore: add the verify-libghostty-vt-node driving skill #25. Now onlyTMPDIRgets the ownership check, and the build already writes compiler temp files there, so the check protects the build too.TMPDIRstill picks the parent directory.Blast Radius
Only agents that run this skill see a change. An agent that passes a
RUN_ROOTpath or sets a supplied-library variable now gets exit 2 and a message that names the fix. The package, its tests, and CI do not use these scripts.Verification
doctor.shexited 0 with a corrupted addon.drive.shran a stale read-only journey file instead of the submitted one and exited 0.launch.shrebuilt libghostty-vt for a suppliedINCLUDE_DIRplusSTATIC_LIB, and failed with exit 1 on a read-only supplied prefix.launch.shexits 2 for each supplied variable, aRUN_ROOTargument, aTMPDIRinside the checkout, and aTMPDIRthat group or others can write (directly, through an ancestor, or through a symlink), and creates nothing.doctor.shexits 1 for a swapped addon (a stripped copy that still loads) and for a corrupted one, and exits 0 after a restore.drive.shexits 2 without running node for an unwritable journey file and for a failedcommand.txtwrite.SKILL.mdon this branch, withTMPDIRunder a sticky1777directory: launch,doctor.shexit 0, 18 of 18 feature journeys exit 0,doctor.shexit 0,cleanup.shexit 0.npm ci,npm run build:libghostty,npm run build:native,npm run build,npm run typecheck,npm test(86 of 86 after the merge of main),npm run smoke. After the merge of main, the 18 journeys include the updatedinfo-fieldscheck ofghosttyCommit.Closes #27
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh