chore: close OSS-readiness Medium + Nice-to-have findings (M1–M13, N1–N13) - #1
Conversation
… add install/uninstall tests
…(M5, N1, N2, N12)
…es (M1, M11-M13, N9)
…icense (M2, M10, N11)
📝 WalkthroughWalkthroughThe PR updates plugin metadata, hardens hook installation and filesystem writes, changes session visibility and logging behavior, and adds CI, tests, and repository documentation/policy files. ChangesRelease, runtime, and repository updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)CHANGELOG.mdmarkdownlint-cli2 wrapper config was not available before execution Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 10-13: Update the CI workflow to use immutable version pins for
the GitHub Actions steps instead of mutable tags, and disable persisted
credentials on the `actions/checkout` step. Adjust the `actions/checkout` and
`actions/setup-node` entries in the workflow so they are locked to fixed
revisions, and set the checkout credentials option off to prevent the repo token
from being written into git config for later steps.
In `@Tests/install.test.mjs`:
- Around line 88-94: The install/uninstall test is not asserting that the
install step succeeded before verifying uninstall behavior. Update the test
around the INSTALL/UNINSTALL calls in the install test to capture and assert the
install result from run(INSTALL, h.home) before proceeding, so failures in
install.js do not get masked by a no-op uninstall. Use the existing run,
INSTALL, UNINSTALL, and readHooks helpers to keep the flow the same while
explicitly checking the install status first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8680dda7-709b-4329-8936-680178435329
📒 Files selected for processing (20)
.codex-plugin/plugin.json.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/pull_request_template.md.github/workflows/ci.yml.swift-formatCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSENOTICEREADME.mdRELEASING.mdSECURITY.mdSources/main.swiftTests/install.test.mjsbuild.shhooks/install.jshooks/lifecycle.jshooks/uninstall.js
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "24" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the actions and turn off persisted checkout credentials.
Lines 10-13 currently use mutable action tags, and actions/checkout keeps the workflow token in git config for later steps by default. That weakens the workflow’s supply-chain posture and already trips the repo’s static checks.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 10-10: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 10-10: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 11-11: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 10 - 13, Update the CI workflow to use
immutable version pins for the GitHub Actions steps instead of mutable tags, and
disable persisted credentials on the `actions/checkout` step. Adjust the
`actions/checkout` and `actions/setup-node` entries in the workflow so they are
locked to fixed revisions, and set the checkout credentials option off to
prevent the repo token from being written into git config for later steps.
Source: Linters/SAST tools
| run(INSTALL, h.home); | ||
| assert.equal(run(UNINSTALL, h.home).status, 0); | ||
| const hooks = readHooks(h.home); | ||
| assert.equal(ourCommands(hooks).length, 0, "no status-bar hooks remain"); | ||
| assert.ok(!("SessionStart" in hooks.hooks), "emptied SessionStart event removed"); | ||
| assert.ok(hooks.hooks.Stop.flatMap((e) => e.hooks).some((h) => h.command === "echo other"), | ||
| "unrelated Stop hook preserved through uninstall"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the install step before checking uninstall behavior.
Line 88 ignores the installer result, so this test can still pass when install.js fails and uninstall.js removes nothing. That hides regressions in the install→uninstall path this case is meant to cover.
Proposed fix
- run(INSTALL, h.home);
+ assert.equal(run(INSTALL, h.home).status, 0);
+ assert.ok(readHooks(h.home).hooks.SessionStart?.length, "install added SessionStart hook");
assert.equal(run(UNINSTALL, h.home).status, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run(INSTALL, h.home); | |
| assert.equal(run(UNINSTALL, h.home).status, 0); | |
| const hooks = readHooks(h.home); | |
| assert.equal(ourCommands(hooks).length, 0, "no status-bar hooks remain"); | |
| assert.ok(!("SessionStart" in hooks.hooks), "emptied SessionStart event removed"); | |
| assert.ok(hooks.hooks.Stop.flatMap((e) => e.hooks).some((h) => h.command === "echo other"), | |
| "unrelated Stop hook preserved through uninstall"); | |
| assert.equal(run(INSTALL, h.home).status, 0); | |
| assert.ok(readHooks(h.home).hooks.SessionStart?.length, "install added SessionStart hook"); | |
| assert.equal(run(UNINSTALL, h.home).status, 0); | |
| const hooks = readHooks(h.home); | |
| assert.equal(ourCommands(hooks).length, 0, "no status-bar hooks remain"); | |
| assert.ok(!("SessionStart" in hooks.hooks), "emptied SessionStart event removed"); | |
| assert.ok(hooks.hooks.Stop.flatMap((e) => e.hooks).some((h) => h.command === "echo other"), | |
| "unrelated Stop hook preserved through uninstall"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/install.test.mjs` around lines 88 - 94, The install/uninstall test is
not asserting that the install step succeeded before verifying uninstall
behavior. Update the test around the INSTALL/UNINSTALL calls in the install test
to capture and assert the install result from run(INSTALL, h.home) before
proceeding, so failures in install.js do not get masked by a no-op uninstall.
Use the existing run, INSTALL, UNINSTALL, and readHooks helpers to keep the flow
the same while explicitly checking the install status first.
- main.swift: collapse the duplicated installer-failure handling into one path; simplify the accessibility label - tests: extract seedHooks helper and OTHER_HOOKS fixture - build.sh: derive VERSION from plugin.json (single source) and use a default-then-guard for SIGN_ID - RELEASING.md: bump version only in plugin.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.sh`:
- Line 8: The VERSION assignment in build.sh can still produce an empty value
when parsing .codex-plugin/plugin.json fails, so add an explicit non-empty
validation immediately after the VERSION line and before any Info.plist
generation. If VERSION is missing or blank, fail the script with a clear error
instead of continuing; keep the check near the existing VERSION parsing logic so
the build stops before writing empty CFBundleVersion fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd80654e-de48-4349-aec7-b3facf2d3c7e
📒 Files selected for processing (4)
RELEASING.mdSources/main.swiftTests/install.test.mjsbuild.sh
✅ Files skipped from review due to trivial changes (2)
- RELEASING.md
- Sources/main.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/install.test.mjs
|
|
||
| APP="build/Codex Status Bar.app" | ||
| BIN="$APP/Contents/MacOS/CodexStatusBar" | ||
| VERSION="${VERSION:-$(sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' .codex-plugin/plugin.json | head -1)}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast if manifest version parsing returns empty.
Line 8 can resolve VERSION to an empty string when .codex-plugin/plugin.json is missing, malformed, or reformatted in a way this sed expression no longer matches, and the script would still emit blank CFBundleVersion fields below. Add an explicit non-empty check before writing Info.plist.
Suggested fix
VERSION="${VERSION:-$(sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' .codex-plugin/plugin.json | head -1)}"
+if [[ -z "$VERSION" ]]; then
+ echo "Failed to determine VERSION from .codex-plugin/plugin.json. Set VERSION=x.y.z explicitly." >&2
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VERSION="${VERSION:-$(sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' .codex-plugin/plugin.json | head -1)}" | |
| VERSION="${VERSION:-$(sed -n 's/.*"version"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' .codex-plugin/plugin.json | head -1)}" | |
| if [[ -z "$VERSION" ]]; then | |
| echo "Failed to determine VERSION from .codex-plugin/plugin.json. Set VERSION=x.y.z explicitly." >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.sh` at line 8, The VERSION assignment in build.sh can still produce an
empty value when parsing .codex-plugin/plugin.json fails, so add an explicit
non-empty validation immediately after the VERSION line and before any
Info.plist generation. If VERSION is missing or blank, fail the script with a
clear error instead of continuing; keep the check near the existing VERSION
parsing logic so the build stops before writing empty CFBundleVersion fields.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hooks/install.js`:
- Line 24: The hook install fingerprint is missing the runtime helper
fs-utils.js, so changes to that file won’t invalidate the installed hook copy.
Update the fingerprinting logic in Sources/main.swift to include fs-utils.js
alongside the other hook resources, and keep the install step in
hooks/install.js aligned with that dependency so any helper change forces a
refresh of the copied hooks.
In `@hooks/update.js`:
- Line 134: The session scanner is picking up the temporary file created by
writeFileAtomic in update.js, which can cause a duplicate session load. Update
Sources/main.swift so the scan for states.d ignores atomic temp files such as
*.tmp (or otherwise excludes writeFileAtomic’s temp naming), keeping only the
real state JSON files in the session list.
In `@Sources/AppSupport.swift`:
- Around line 28-32: The append path in AppSupport.swift updates permissions
after writing, so an existing permissive log can receive new data before being
locked down. In the log-writing flow around the FileHandle(forWritingAtPath:)
branch, set the file’s permissions to 0o600 before seekToEndOfFile/write when
app.log already exists, and keep the existing permission-setting logic for newly
created files as well. Use the existing handle/fm logic in this helper to ensure
old logs are chmod’d before any append occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e244cd9-257b-404a-bc42-c04fa0545958
📒 Files selected for processing (15)
.codex-plugin/plugin.json.github/workflows/ci.ymlCHANGELOG.mdSources/AppSupport.swiftSources/SessionState.swiftSources/main.swiftTests/install.test.mjsTests/logic_tests.swiftTests/update.test.mjsbuild.shhooks/fs-utils.jshooks/install.jshooks/lifecycle.jshooks/uninstall.jshooks/update.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- hooks/uninstall.js
- build.sh
| ensurePrivateDir(sbDir); | ||
| copyPrivateFile(path.join(__dirname, "update.js"), updateDest); | ||
| copyPrivateFile(path.join(__dirname, "lifecycle.js"), lifecycleDest); | ||
| copyPrivateFile(path.join(__dirname, "fs-utils.js"), fsUtilsDest); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include fs-utils.js in the hook install fingerprint.
Line 24 installs a runtime dependency used by copied hooks, but Sources/main.swift fingerprints only the other hook resources. A fs-utils.js-only change can leave users running the stale copied helper.
Suggested fix
- let names = ["install.js", "update.js", "lifecycle.js", "uninstall.js"]
+ let names = ["install.js", "update.js", "lifecycle.js", "uninstall.js", "fs-utils.js"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/install.js` at line 24, The hook install fingerprint is missing the
runtime helper fs-utils.js, so changes to that file won’t invalidate the
installed hook copy. Update the fingerprinting logic in Sources/main.swift to
include fs-utils.js alongside the other hook resources, and keep the install
step in hooks/install.js aligned with that dependency so any helper change
forces a refresh of the copied hooks.
| fs.renameSync(tmp, statePath); | ||
| ensurePrivateDir(dir); | ||
| ensurePrivateDir(statesDir); | ||
| writeFileAtomic(statePath, JSON.stringify(out)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep atomic temp files out of the session scanner.
writeFileAtomic creates a complete *.tmp next to statePath; Sources/main.swift currently scans every file in states.d, so it can briefly load the temp JSON as a duplicate session.
Suggested fix
if let names = try? fm.contentsOfDirectory(atPath: statesDir) {
for name in names {
+ if name.hasSuffix(".tmp") { continue }
let p = (statesDir as NSString).appendingPathComponent(name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hooks/update.js` at line 134, The session scanner is picking up the temporary
file created by writeFileAtomic in update.js, which can cause a duplicate
session load. Update Sources/main.swift so the scan for states.d ignores atomic
temp files such as *.tmp (or otherwise excludes writeFileAtomic’s temp naming),
keeping only the real state JSON files in the session list.
| if let handle = FileHandle(forWritingAtPath: path) { | ||
| handle.seekToEndOfFile() | ||
| handle.write(data) | ||
| handle.closeFile() | ||
| try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Chmod existing logs before appending.
If app.log already exists with permissive permissions from an older version, Line 30 writes the new timeout line before Line 32 restricts the file.
Suggested fix
+ if fm.fileExists(atPath: path) {
+ try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: path)
+ }
if let handle = FileHandle(forWritingAtPath: path) {
handle.seekToEndOfFile()
handle.write(data)
handle.closeFile()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let handle = FileHandle(forWritingAtPath: path) { | |
| handle.seekToEndOfFile() | |
| handle.write(data) | |
| handle.closeFile() | |
| try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: path) | |
| if fm.fileExists(atPath: path) { | |
| try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: path) | |
| } | |
| if let handle = FileHandle(forWritingAtPath: path) { | |
| handle.seekToEndOfFile() | |
| handle.write(data) | |
| handle.closeFile() | |
| try? fm.setAttributes([.posixPermissions: 0o600], ofItemAtPath: path) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppSupport.swift` around lines 28 - 32, The append path in
AppSupport.swift updates permissions after writing, so an existing permissive
log can receive new data before being locked down. In the log-writing flow
around the FileHandle(forWritingAtPath:) branch, set the file’s permissions to
0o600 before seekToEndOfFile/write when app.log already exists, and keep the
existing permission-setting logic for newly created files as well. Use the
existing handle/fm logic in this helper to ensure old logs are chmod’d before
any append occurs.
1 participant
Summary
Closes the open Medium (M1–M13) and Nice-to-have (N1–N13) findings from the 2026-06-26 OSS-readiness audit. No new runtime dependencies; changes are surgical and the project's zero-dependency property is preserved. (High-severity Homebrew items H4/H5 are intentionally out of scope.)
Plan:
~/agent-docs/codex-status-bar/plans/2026-06-26-oss-readiness-mediums-nice-to-haves.md.What changed
App / hooks (Swift + JS)
NSAlertwhen the bundledinstall.jsfails, and prepends/opt/homebrew/bin:/usr/local/binto broaden node resolution; removed the deadownerExited(_:)and the write-onlyinstalledVersiondefault.install.jsnow quotes the interpolatednodeand script paths (safe under a$HOMEwith spaces).execFileSyncforpgrep(lifecycle) andpkill(uninstall).com.local.codexstatusbar→io.github.kiwigaze.codexstatusbar(build.sh + lifecycle.js).Build
build.shgains aVERSIONvar with an interpolated Info.plist heredoc, and guards an emptyTEAM_IDso it can no longer sign with an unrelated Developer ID.Tests / CI
Tests/install.test.mjscovering the hook-merge logic (idempotency, only-our-hooks removal, one-time.bak-statusbar)..github/workflows/ci.yml(macOS build + Swift logic tests + Node tests +node --check) and a.swift-formateditor-assist config. No Swift lint gate: the hand-tuned compact style cannot pass swift-format without a disruptive reformat (verified), so the gate would be noise.Docs / metadata
SECURITY.md(private reporting + an accurate account of every shell-out the hooks and the app make)..codex-plugin/plugin.jsonversion0.2.2+ homepage/repository filled; author set toYijiazhen Qiin LICENSE, NOTICE, and the manifest.NOTICE, which already ships into the app + DMG, so it travels with the artifact.[0.2.0]entry + Keep-a-Changelog reference.state.jsonreference.CONTRIBUTING.md,CODE_OF_CONDUCT.md(Contributor Covenant 2.1, GitHub-profile contact), issue + PR templates.RELEASING.md.Repo-settings actions (done outside this PR)
Verification
Local CI gate is green:
node --test Tests/install.test.mjs Tests/update.test.mjs(all pass), Swift logic tests (ALL OK),node --checkon all hooks, andSKIP_NOTARIZE=1 ./build.shproduces a universal (x86_64 arm64) binary.Review
Branch was reviewed (code-correctness + docs-accuracy passes); findings were addressed — most notably the CI Node version was set to
24so theusing/Symbol.disposetest syntax parses, andSECURITY.mdnow discloses the app's own/bin/zsh -lc/osascript/pgrepcalls.Summary by CodeRabbit