Validate --interval-seconds before writing the launch agent

[devYRPauli]

Follow-up to #133, which added the numeric option helpers this uses.

Problem

jobs account-sync install and jobs bookmark-sync install both take --interval-seconds and pass it through unchecked:

intervalSeconds: Number(options.intervalSeconds),

The library default cannot catch a bad value. src/lib/account-sync-job.ts:475 and src/lib/bookmark-sync-job.ts:298 use:

options.intervalSeconds ?? DEFAULT_ACCOUNT_SYNC_INTERVAL_SECONDS

?? replaces only null and undefined. NaN is neither, so it passes through to src/lib/launchd.ts:141:

<integer>${String(intervalSeconds)}</integer>

birdclaw jobs account-sync install --interval-seconds abc therefore writes <integer>NaN</integer>. macOS rejects that plist:

$ plutil -lint nan.plist
nan.plist: (Unknown character 'N' (0x4e) in <integer> on line 8)

The command reports success and the agent is written, but it never runs on a schedule.

Change

Both actions now parse the option first with parsePositiveIntegerOption, the helper added in #133, and return before any install work when it is invalid. An interval must be at least 1, which matches what StartInterval accepts.

src/lib/launchd.ts, account-sync-job.ts and bookmark-sync-job.ts are unchanged. The validation sits at the CLI boundary, the same place as #132 and #133. No flag name, default value or help text changes.

Tests

Two tests in src/cli.test.ts, one per command. Each asserts that an invalid interval exits 1 and that no plist is installed, and that a valid interval still reaches the installer with the right number.

node ./node_modules/vitest/vitest.mjs run src/cli.test.ts
Tests  95 passed (95)

npx tsc --noEmit
exit 0

Reverting only the source change and keeping the tests fails exactly the two new ones:

* rejects an invalid account sync interval before installing a plist
* rejects an invalid bookmark sync interval before installing a plist
Tests  2 failed | 93 passed (95)

Real behavior proof

Built the CLI with npm run build:node and ran the real binary against a temporary LaunchAgents directory, so nothing touches the real one.

Before, on main's handler:

$ node bin/birdclaw.mjs jobs install-account-launchd \
    --interval-seconds abc --launch-agents-dir /tmp/bcla2 --no-load
  "ok": true,

$ grep -A1 StartInterval /tmp/bcla2/com.steipete.birdclaw.account-sync.plist
  <key>StartInterval</key>
  <integer>NaN</integer>

$ plutil -lint /tmp/bcla2/com.steipete.birdclaw.account-sync.plist
  (Unknown character 'N' (0x4e) in <integer> on line 27)

The command reports success and writes a plist that macOS cannot parse.

After, on this branch:

$ node bin/birdclaw.mjs jobs install-account-launchd \
    --interval-seconds abc --launch-agents-dir /tmp/bcla --no-load
{"error":"--interval-seconds must be a non-negative integer"}

$ ls /tmp/bcla | wc -l
0

No plist is written. A valid value is unaffected:

$ node bin/birdclaw.mjs jobs install-account-launchd \
    --interval-seconds 1800 --launch-agents-dir /tmp/bcla --no-load
  "ok": true,
$ grep -A1 StartInterval /tmp/bcla/com.steipete.birdclaw.account-sync.plist
  <key>StartInterval</key>
  <integer>1800</integer>

install-bookmarks-launchd takes the same option through the same helper.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-31T16:26:28.965850Z	8500297	PR opened

ℹ️ 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" or "@codex security review".

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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8500297f1a

ℹ️ 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".

[chatgpt-codex-connector]

Keep accepting Number-compatible interval spellings

When an existing script supplies an interval such as 1e3, +60, or 0x3c, the previous Number(options.intervalSeconds) conversion produced a valid positive integer and a valid plist, but parsePositiveIntegerOption delegates to a decimal-digits-only regex and now rejects these inputs. This changes the accepted CLI grammar while attempting only to reject invalid intervals; validate the converted number as a safe positive integer (as parseLimitOption does for compatibility) for both launchd commands.

Useful? React with 👍 / 👎.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 31, 2026, 3:08 PM ET / 19:08 UTC.

ClawSweeper review

What this changes

This PR validates positive launchd intervals for account and bookmark sync installers before any plist is written, while retaining Number-compatible inputs such as 1e3.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open: the focused CLI fix appears correct, preserves the previously accepted numeric grammar, and has sufficient real-command proof; it is ready for ordinary maintainer review.

Priority: P2
Reviewed head: 16dd70b634ed0977924dec86a913df161f04f8b8

Review scores

Measure	Result	What it means
Overall readiness	🦞 diamond lobster (5/6)	Strong terminal before/after evidence, focused coverage, and the resolved compatibility concern make this a high-confidence fix.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The changed production owner is the jobs CLI installer path; the contributor rebuilt the CLI and ran both before/after scenarios against a temporary LaunchAgents directory, observing an invalid-plist write before the change and a structured error with zero written plists after it, while a valid interval still produced the expected value.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The changed production owner is the jobs CLI installer path; the contributor rebuilt the CLI and ran both before/after scenarios against a temporary LaunchAgents directory, observing an invalid-plist write before the change and a structured error with zero written plists after it, while a valid interval still produced the expected value.
Evidence reviewed	6 items	Introduced CLI validation: Both LaunchAgent install actions now validate the interval and return before calling their installer when the value is invalid. Compatibility preserved: The new parser builds on the existing Number()-based limit parser, rejecting non-positive values while retaining valid scientific and hexadecimal spellings. Underlying failure path: The plist builder serializes the supplied interval directly inside StartInterval, so an unchecked NaN would produce an invalid scheduler plist.
Findings	None	None.
Security	None	None.

How this fits together

Birdclaw’s jobs CLI turns scheduled-sync options into macOS LaunchAgent plists. The interval value flows from a user command through validation into the plist consumed by launchd.

flowchart LR
A[User interval option] --> B[Jobs install command]
B --> C[Positive numeric validation]
C -->|invalid| D[Structured CLI error]
C -->|valid| E[LaunchAgent plist builder]
E --> F[macOS launchd scheduler]

Loading

Before merge

• Complete next step (P2) - No repair lane is needed; the patch has no remaining actionable finding and awaits routine maintainer merge review.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test scope	production +38/-3, tests +106	The small shared-parser and two-command change has direct regression coverage for both installer paths.

Technical review

Best possible solution:

Merge the CLI-boundary validation while retaining the established Number-compatible interval syntax.

Do we have a high-confidence way to reproduce the issue?

Yes—the base conversion and plist serialization establish the path, and the supplied rebuilt-CLI terminal evidence shows invalid input previously wrote NaN while this branch writes no plist.

Is this the best way to solve the issue?

Yes—the validation is placed before the installer side effect and uses the existing compatible numeric grammar instead of changing defaults or library behavior.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 73303adbc8c0.

Labels

Label justifications:

• P2: This fixes invalid CLI input that can create a nonfunctional scheduled job, with blast radius limited to the two optional launchd installers.
• rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the jobs CLI installer path; the contributor rebuilt the CLI and ran both before/after scenarios against a temporary LaunchAgents directory, observing an invalid-plist write before the change and a structured error with zero written plists after it, while a valid interval still produced the expected value.
• proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the jobs CLI installer path; the contributor rebuilt the CLI and ran both before/after scenarios against a temporary LaunchAgents directory, observing an invalid-plist write before the change and a structured error with zero written plists after it, while a valid interval still produced the expected value.

Evidence

What I checked:

• Introduced CLI validation: Both LaunchAgent install actions now validate the interval and return before calling their installer when the value is invalid. (src/cli/register-jobs.ts:101, 16dd70b634ed)
• Compatibility preserved: The new parser builds on the existing Number()-based limit parser, rejecting non-positive values while retaining valid scientific and hexadecimal spellings. (src/cli/command-context.ts:155, 16dd70b634ed)
• Underlying failure path: The plist builder serializes the supplied interval directly inside StartInterval, so an unchecked NaN would produce an invalid scheduler plist. (src/lib/launchd.ts:141, 16dd70b634ed)
• Focused regression coverage: Parameterized CLI tests cover both installers for invalid input, a normal valid input, and preserved 1e3 parsing. (src/cli.test.ts:795, 16dd70b634ed)
• Feature-history routing: Current-main history identifies Peter Steinberger as the original author of both scheduled bookmark and account launchd jobs and a recent contributor to CLI numeric-parser compatibility. (src/cli/register-jobs.ts:63, f2531ea91c18)
• Current branch and release state: The fix is introduced only by this open branch against current main and is not contained by a local release tag. (16dd70b634ed)

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)

• reviewed 2026-08-31T17:08:31.208Z sha 8500297 :: needs real behavior proof before merge. :: [P1] Preserve Number-compatible positive interval spellings
• reviewed 2026-08-31T17:42:28.456Z sha 24e1ef4 :: needs changes before merge. :: [P1] Preserve Number-compatible positive interval spellings
• reviewed 2026-08-31T17:55:47.893Z sha 16dd70b :: needs maintainer review before merge. :: none

[devYRPauli]

@clawsweeper re-review

Two things landed since the last review.

Bun primary was red on formatting, not tests. src/cli.test.ts was not oxfmt formatted. Fixed in 24e1ef4. oxfmt --check now passes on all 399 files, oxlint is clean and tsc --noEmit exits 0.

Real behavior proof added to the PR body. I built the CLI with npm run build:node and ran the real binary against a temporary LaunchAgents directory.

Before, on main's handler, the command reports success and writes a plist macOS cannot parse:

  "ok": true,
  <integer>NaN</integer>
plutil -lint: (Unknown character 'N' (0x4e) in <integer> on line 27)

After, on this branch:

{"error":"--interval-seconds must be a non-negative integer"}
plists written: 0

A valid --interval-seconds 1800 still installs and writes <integer>1800</integer>.

One note on method: bin/birdclaw.mjs runs dist/cli/birdclaw.js, so a first attempt at this transcript ran the stale bundle and looked like the fix did nothing. The numbers above are from a rebuilt dist.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/33421200700
• Updated: 2026-08-31T17:44:51.500Z

[devYRPauli]

Fixed in 16dd70b. You were right, and I picked the wrong helper.

parsePositiveIntegerOption tests the raw string with /^\d+$/, so it rejects 1e3 and 0x10. Number() accepted both before this branch, so those spellings regressed.

parseLimitOption already documents the correct approach in its own comment: keep the Number() grammar and validate the result. I added parsePositiveLimitOption, which mirrors parsePositiveIntegerOption on top of parseLimitOption, and used it for both install commands.

Real CLI, freshly built dist:

--interval-seconds 1e3   ->  "ok": true      <integer>1000</integer>
--interval-seconds abc   ->  {"error":"--interval-seconds must be a non-negative integer"}   0 plists
--interval-seconds 0     ->  {"error":"--interval-seconds must be at least 1"}

0 stays rejected because StartInterval needs at least 1.

Two parameterised tests cover the preserved spellings, one per command. Reverting to parsePositiveIntegerOption fails exactly those two and leaves the other 95 passing.

node ./node_modules/vitest/vitest.mjs run src/cli.test.ts
Tests  97 passed (97)

oxfmt --check   all matched files use the correct format
oxlint          clean
tsc --noEmit    exit 0

[steipete]

LAND recommended for 16dd70b634ed0977924dec86a913df161f04f8b8 after independent local verification. The CLI validation is in the right place: Number("abc") becomes NaN, the library's nullish default preserves it, and the plist serializer writes an invalid integer. Both installers now reject that value before installation while retaining Number-compatible spellings.

I prepared the missing documentation and changelog entry in 51dfbdc, with credit to @devYRPauli. The maintainer candidate branch contains this PR plus that documentation-only commit. The original fork is unchanged. The landing owner can include that commit when landing this PR.

Built-CLI proof on macOS, using temporary BIRDCLAW_HOME and LaunchAgents directories with --no-load:

./scripts/bun-canary.sh install --frozen-lockfile
./scripts/bun-canary.sh scripts/build-cli.mjs
proof=$(mktemp -d)
BIRDCLAW_HOME="$proof/home" node bin/birdclaw.mjs jobs install-account-launchd \
  --interval-seconds abc --launch-agents-dir "$proof/agents" --no-load

Repeated for install-bookmarks-launchd; after rebuilding the PR, exercised both commands under Node 26.8.1 and the pinned Bun canary (./scripts/bun-canary.sh bin/birdclaw.mjs ...). Each invocation used a fresh temporary directory. Checked every successful output with /usr/bin/plutil -lint <plist> and parsed its StartInterval.

Case	Current main 73303ad	PR head
abc, both installers	Exit 0, ok: true, writes <integer>NaN</integer>; plutil exits 1 with Unknown character 'N' (0x4e) in <integer>	Exit 1, {"error":"--interval-seconds must be a non-negative integer"}, zero plists
0	—	Exit 1, {"error":"--interval-seconds must be at least 1"}, zero plists
-1, 1.5, NaN, Infinity, 9007199254740992	—	Exit 1, structured error, zero plists
1800, 1e3, 0x10, +60, 1	—	Exit 0; valid plist intervals 1800, 1000, 16, 60, 1
Omitted interval	—	Exit 0; account default 1800, bookmarks default 10800; valid plists

Result: 52 real built-CLI invocations passed across both runtimes. No scheduled jobs were loaded.

./scripts/bun-canary.sh ./scripts/run-vitest.mjs run \
  src/cli.test.ts src/lib/account-sync-job.test.ts \
  src/lib/bookmark-sync-job.test.ts src/lib/launchd.test.ts
# Test Files 4 passed (4); Tests 128 passed (128)

./scripts/bun-canary.sh run --bun check
# Format, lint, and typecheck passed.

git diff --check
# Passed.

Codex autoreview of the PR plus documentation was scoped-clean at the helper's default P0 threshold. Manual review also checked the previous numeric-grammar finding, installer ordering, defaults, and the regression tests; no remaining actionable defect found.

CI run 33421817650 is green for all four CI lanes on the original PR head; GitGuardian also passed. The earlier formatting and numeric-grammar problems are resolved. The documentation-only candidate commit has local validation; the non-main branch push does not trigger this repository's CI workflow.

No merge, release, or closure performed.

[clawsweeper]

ClawSweeper status: review started.

I am starting a fresh review of this pull request: Validate --interval-seconds before writing the launch agent This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.