Conversation
* fix(officecli): preflight Windows release assets * test(officecli): gate PowerShell cases to Windows
#308) Control was gated to darwin since the skill was introduced, and the prompts taught get_app_state as the universal first call. Computer Use now runs on Windows, where the API is window-scoped: list_windows() then get_window_state({app,id}), with no get_app_state and no select_text. An agent following the old contract gets 'sky.get_app_state is not a function'. - runtimeHints gains supportedPlatforms; Control declares darwin+win32. requiresDarwin stays because runtimeHints is serialized through GET /api/employees, and is now DERIVED so the two cannot disagree. - Linux and WSL stay denied: a WSL process reports linux via process.platform and is refused by omission from the allowlist. - Prompts document the two Windows results that look like success: list_apps() answers with a dead pipe, and an empty list_windows() means you are not on the pipe rather than that no windows are open. - The desktop-control anchor can now be migrated in already-installed A-1 files, including the pre-hash customized path that previously kept a stale block forever. Replacement is gated on a committed hash of the block we shipped, so user text inside the markers is never destroyed, and malformed or duplicated markers preserve the file with a warning.
β¦d docs (#308) - skills_ref gitlink: desktop-control drops the macOS-only gate and documents the window-scoped Windows API, the two false-success traps, and the logged-on-session/node_repl/sandbox preconditions. - README no longer tells Windows users Computer Use is macOS-only. - A-1 keeps only the routing decision; the pipe/config.toml/SSH depth lives in the skill. Budget raised 36,000 -> 37,100 with the reason recorded in the test, since Windows is a different API surface rather than a variant. - Contract tests pin the skill and registry so this cannot regress silently.
β¦inned Locks in TTL clamping, negative-cache classes and per-id keying, the shared capability latch across users.info/bots.info, single-probe admission after the lock lapses, in-flight slot release after failure, partition independence, workspace-scoped priming, batch cap/partial reporting, and reset coverage. These describe what identity.ts does today, not what it should do. They exist so the enrichment-cache extraction (devlog 260812_slack_conversation_context/012) cannot silently change behavior β an independent audit enumerated these as gaps the existing 43 tests leave open. 13 pass against the unmodified tree.
TTL cache with cap eviction, classified failure suppression (resource vs capability keys), capability lockout with a single admitted re-probe, in-flight coalescing, aggregate cancellation, and generation invalidation. All of this already existed inside identity.ts. Specifying it a second time for conversation lookups failed four consecutive audit rounds, because the second spec kept disagreeing with the first about cancellation ownership β slackApi composes the caller signal at dispatch (api.ts:116-131), so a solo request can never be promoted to shared. Extracting it means the discipline is written once. Two invariants the tests caught during implementation: - the expired-capability check must be inline, not via isSuppressed(), because that helper deletes the expired key as a side effect and so erased the marker deciding who becomes the single probe β admitting every caller instead of one - release() must not abort a settled record: the last waiter also departs on the success path, and aborting there fires cancellation handlers for a request that already delivered Generation gates BOTH the cache write and the publication to waiters, which is stricter than identity.ts today (it skips the write but still returns the value, leaking one stale name per reset boundary). That fix lands with the rewire. 19 primitive tests; full slack suite 385 pass / 0 fail (baseline 353/0).
identity.ts no longer owns its TTL maps, negative cache, capability latch, in-flight coalescing, generation counter, or signal racing β the shared primitive does. What stays is what is genuinely Slack-identity-specific: the users.info/bots.info calls, name precedence and sanitization, the degraded() inline-name fallback, the once-per-process missing-scope warning, and the roster top-up pacing. Behavior is preserved deliberately: - ONE capability key for both users.info and bots.info, keeping today's shared latch rather than splitting per method - separate user/bot partitions so each keeps its own CACHE_CAP; merging them would halve capacity and change eviction order - the not_found vs transient negative-TTL split is expressed through the failure classifier Evidence: the 43 existing identity tests pass UNMODIFIED, plus the 13 characterization tests added first. 632 -> 553 lines. Full slack suite 385 pass / 0 fail (baseline 353/0), build exit 0.
β¦312) The Settings page read /api/cli-status once at mount and rendered the notice straight from that snapshot, so a probe still running at mount left 'μν νμΈ μ€' on screen indefinitely. The server cannot resolve this alone: CliStatusCache is demand-driven and has no timer, so a snapshot only advances when somebody reads it again. The UI is now that somebody, bounded two ways. - failing is deliberately NOT terminal. The cache resumes probing on the first read after its backoff expires, so stopping there would have swapped one permanent notice for another and never shown the recovery. Polling continues through failing while honoring the server's nextRetryAt. - Two independent bounds that never reset on a server response: a wall-clock horizon and a cap on real requests. Waiting out a backoff does not consume an attempt, so a host that is merely backing off is not declared timed out. The horizon outlives the 60s worker timeout so a healthy slow probe is not misreported. - Exhaustion renders its own notice. Left to the snapshot alone it would still say 'checking' and reproduce the reported symptom exactly. - Responses are guarded by a generation ref (the Browser.tsx convention): clearing a timer does not stop an in-flight request from calling setState after unmount or after the user switches CLI.
conversation.ts answers 'which conversation is this, and who is taking part', consuming the shared enrichment-cache rather than reimplementing its discipline. Slack-specific decisions worth naming: - participants are derived from message AUTHORS, never reply_users β Slack's own reference warns that field 'sometimes contains bot IDs rather than user IDs' - bot markers win over user, because a granular-permission app message carries both; bots are included as participants (isBot), since a bot-only thread reporting 'no participants' would be false - channel-scoped permission errors (no_permission, access_denied, channel_not_found) suppress per channel, NOT workspace-wide: locking the method because one private channel is unreadable would create the outage the lock exists to prevent - names and topics pass the same sanitizer as display names, so a topic cannot forge a prompt line - num_members is labelled as Slack's reported member count; Slack does not document a bot/human split, so none is claimed - conversations.info starts are gated to one per 1.2s (Tier 3 is 50+/min; 1/s would allow 60 and exceed the documented floor) and a declined start degrades immediately rather than queueing behind cold-channel demand history.ts gains optional signal propagation and an opt-in noRetryOnRateLimit. Both default to today's behavior, so the /api/slack/history route and attachment recovery are untouched; only enrichment opts out of retrying a 429, because it applies its own suppression window. 24 conversation tests; slack suite 412 pass / 0 fail; build exit 0.
β¦ state
Both adapters passed the resource key into classifyFailure through a
module-level variable, set before the load and re-set after it. That is not
safe: two concurrent loads on different keys interleave, and a slow continuation
classifies its failure AFTER a peer has overwritten the variable.
Reproduced standalone before fixing β load KEY_A failing suppressed KEY_B
instead. Consequence in production: the innocent channel/user goes quiet for the
suppression window while the actually-broken one keeps hammering Slack, which is
the exact failure the suppression exists to prevent.
classifyFailure now receives an explicit FailureContext { resourceKey,
capabilityKey } and neither adapter keeps mutable module state.
The regression test is ablation-verified: reintroducing a stale key makes it
fail with 'err_a must suppress KEY_A', and the fix makes it pass.
slack suite 413 pass / 0 fail; build exit 0.
β¦ache Audit findings on the WP1b diff, all accepted: - conversations.info and conversations.replies shared one capability key and one 1.2s start clock. They are separate Tier-3 methods needing different scopes, so a missing_scope on info locked replies for 30 minutes, and a channel lookup starved the thread lookup that follows it on the same message. Both are now keyed per method. - replyCount reported the fetched-window size, so a 500-reply thread read as 49. Slack's reply_count on the parent is authoritative; the window is only a fallback. The old test asserted length-1 and would have passed either way. - the thread cache retained full message bodies: 500 entries x 50 messages x Slack's 40k-character limit is a multi-gigabyte ceiling. Retained text is now capped per message and file metadata is dropped. - a stale missing_scope (issued under the previous token) could re-arm the warn-once latch that reset had just cleared, silencing the real warning for the new workspace. The latch now only accepts a current-generation failure. - str_func.md registers both new modules and the line counts are back in sync. slack suite 418 pass / 0 fail; build exit 0; verify-counts exit 0 (418 items).
The test asserted that the next lookup still ran, which stays true even when a stale response silently consumes the warn-once latch β precisely the bug it was meant to catch. It now asserts the latch state directly through a narrow test-only accessor, since the latch is otherwise unobservable. Ablation-verified: removing both generation guards makes it fail with 'a superseded response must not consume the warn-once latch'. slack suite 418 pass / 0 fail; build exit 0; verify-counts exit 0.
β¦hell rules (#302, #310) Two halves of the same defect: the rule existed where it was not needed and was missing where it was. Code half. cli-jaw does author PowerShell β scripts/install.ps1 and scripts/install-officecli.ps1 are checked in and published through the package.json files list β and both shipped without a BOM. The officecli installer prints non-ASCII status symbols, so on a Korean host PowerShell 5.1 decodes them as CP949 and U+2714 arrives as a different character entirely. Both files now start with EF BB BF; nothing else about them changed. Guidance half. ba1c157 recorded the shell hazards in the repo AGENTS.md, which guides agents developing cli-jaw. The agent cli-jaw dispatches onto a user's Windows host never reads that file, and the runtime prompts carried no Windows shell guidance at all. The invariant now lives in a1-system.md (Boss) and employee.md (dispatched workers) β not in a skill, because writing a .ps1 is plain scripting work that never routes through desktop-control. Recorded there: the BOM requirement and its CP949 mechanism, LEN as the only diagnostic that separates corrupted data from a garbled console, the three shells and the DefaultShell key that picks between them, and the nested-shell trap where the outer shell expands the variables first. A-1 budget 37,100 -> 37,800, reason recorded in the test.
The 1s backoff used an unref'd timer, so a process with nothing else pending could exit mid-await and the promise would resolve never. Under CI load that surfaced as 'Promise resolution is still pending but the event loop has already resolved' across the whole slack-history file. The unref'd default is right for a cancelled ingress that must not hold the loop open, so it stays the default; only the awaited backoff opts in.
) The ingress dedupe was a process-memory Map while the agent queue is a SQLite table. A reconnect before Slack observed our ACK could therefore admit the same delivery twice under the next lifecycle, and the downstream dedupKey cannot catch it because that key hashes the prompt body β enrichment that resolves on one delivery and degrades on the next splits the key. Ordering is the whole design here. Committing durably at the reservation site would have been worse than the bug: the socket acks before any work, and an await plus five early returns sit between the reservation and admission, so a durable claim there converts a recoverable redelivery into ten minutes of silent message loss. Duplication is visible and cancellable; a vanished message is not. So the reservation stays in memory, exactly where it was, and the durable commit happens only after a run is admitted, with no await in between. An event that died before admission stays redeliverable. A reset between reserve and admit reopened the same double-admission window -- memory is cleared, a redelivery re-reserves, and the original handler wakes up and admits into the new generation. The reservation now captures the ingress generation and revalidates it immediately before admission. Durable commit failure is fail-open: the run is already accepted, and cancelling it to protect a bookkeeping row would risk losing the message.
β¦context # Conflicts: # structure/str_func.md
The redaction-sinks gate caught these: a raw error message from the dedupe store can carry connection or credential text, and every channel logger is required to mask before it writes. Uses logErrorText like the rest of the Slack path rather than taking an allowlist exemption.
* docs: explain native Windows log ownership * docs: harden Windows log lifecycle examples
fix(slack): preserve active prefetch claims under cap
safeResolveUnder and assertSendFilePath compared resolved paths with raw case-sensitive startsWith. Two consequences on Windows: - 'a.md:hidden' passed containment. Verified on a real Windows host that this writes an NTFS alternate data stream: absent from a name-only directory listing, yet fully readable. Decisively, fs.realpathSync.native RESOLVES the ADS form, so a check placed after canonicalization would let it through -- the rejection has to happen before. Also verified 'trailing.md.' lands on disk as 'trailing.md', so two distinct strings name one file. - 'C:\Data' and 'c:\data' compared unequal while naming one directory. Adds an injectable PathEnvironment so Windows rules are testable on any CI OS, matching how platform-kind.ts takes its inputs as parameters. Folding is for comparison only -- the unfolded path is still returned -- and is ASCII-only to avoid the locale-sensitive Turkish dotless-i hazard. POSIX is untouched: a colon is a legal filename character there, and case sensitivity is preserved. Ordinary '..' traversal was NOT broken before this change and still is not. A first implementation of the trim rule rejected '..' itself, which the new tests caught; dot segments are now exempt and containment alone decides escape. 23 new tests, all constructible on macOS via injected win32/posix semantics, plus real-Windows verification of the shipped rule. Zero new failures against the pinned baseline. Plan: devlog/_plan/260812_windows_and_channels_parity/010
β¦ve NTFS An adversarial audit found my own previous commit introduced a bypass, and proved it on a real Windows host. Windows supports per-directory case sensitivity (fsutil file setCaseSensitiveInfo) and case-sensitive SMB shares, so C:\...\Root and C:\...\root can be two DIFFERENT directories. Folding ASCII merely because the host is win32 made a forbidden sibling look contained. Measured on the host, same fixture: OLD_FOLDING_ALLOWS=true <- the bypass NEW_EXACT_ALLOWS=false <- closed CI_NEW_EXACT_ALLOWS=true <- and no false denial on a normal volume The correct source of case-insensitivity is native realpath, which restores each entry's true on-disk casing; comparing those canonical forms exactly is right on both volume types. safeResolveUnder is purely lexical and cannot know the volume type, so it now compares exactly and fails closed. Two more audit findings fixed: - \?\C:\... and \.\C:\... were rejected as ADS because the drive colon sits in the ROOT. Parse the root with path.win32.parse and scan only the tail, as the plan originally specified. - projectDirs required realpath(dir) === resolve(dir), which on Windows never held when stored casing differed from on-disk casing (verified: '...\mixed' canonicalizes to '...\MiXeD'), silently dropping the root. Tests now model realpath honestly: case-insensitive lookup returning TRUE on-disk casing, plus a case-sensitive mode. The earlier fake echoed the input back, which is what hid these bugs. 27 tests, zero new failures vs baseline (the capability-probe failure is pre-existing flake under parallel load: 3/3 green in isolation and it never touches path-guards). Plan: devlog/_plan/260812_windows_and_channels_parity/010
Adds an idempotent OwnedProcess lifetime wrapper to process-kill.ts that composes over the existing tree helpers. Every live-child termination path now routes through it: duplicate-registration reaper, killActiveAgent, killAllAgents, notes search (timeout/output-limit/completion), bgtask runner (stall/cancel/shutdown/completion), ACP host (handshake/idle/dispose), ACP client, and capabilities probe. The three independent escalation machines in spawn.ts each had their own timer+guard logic. Two guarded on ChildProcess.killed, which the codebase itself documents as unreliable (a CLI that traps SIGTERM stays alive with killed set). The owner re-checks exitCode/signalCode instead. Key invariants: - PID captured once at construction, never retargeted after a grace period - First termination reason wins; terminate() and complete() are idempotent - ownProcess() is memoized by ChildProcess identity so owners cannot compete - Escalation re-checks the ORIGINAL child before firing Honest limit: Node cannot spawn suspended and assign a Job Object, so a descendant that escapes before the first tree walk is not guaranteed contained (openai/codex closes this with a native helper). 37 new tests (15 core + 15 routing + 7 updated existing). Zero new failures vs pinned baseline. verify-counts 419/419. Plan: devlog/_plan/260812_windows_and_channels_parity/050
Adds DeliveryFailure type extending the existing SendFailureKind with auth, permission, not-found, and transient. The critical distinction: 'ambiguous' means the provider may have accepted the message (no retry), while 'transient' requires affirmative evidence that no request was dispatched (safe to retry). Three concrete mappers: telegramDeliveryError wraps the existing classifier preserving its semantics bit-for-bit, slackDeliveryError maps Slack error codes, discordDeliveryError maps HTTP status codes. Also adds ChannelCapabilities with verified constants (Slack 3900, Discord 2000, Telegram 32000 rich / 4096 plain) and fixes slackApi() to preserve the Retry-After header instead of discarding it. 6 new tests. Plan: devlog/_plan/260812_windows_and_channels_parity/060
β¦etection Adds WindowsShellKind type and detectWindowsShell() with injectable probes. Detection order: pwsh.exe first (modern), powershell.exe (legacy 5.1), Git Bash common paths, cmd.exe fallback. Updates Electron shell-discovery to use the taxonomy instead of a hard-coded ['powershell.exe','cmd.exe'] list, and fixes bootstrap shell recording to use the detector on win32 instead of the always-empty SHELL env var. Note: electron/src/main/lib/terminal/index.ts still passes ['-l'] to all shells β the interactive argv fix is a follow-on. 6 new tests, 72 platform tests green. Plan: 040
Extracts the rate-limited one-message-per-run streaming pattern from Slack progress.ts into a generic draft-stream module. Slack's public SlackProgressHandle API preserved β it delegates to the shared DraftStream. Key semantics: coalescing (latest-wins, not a queue), finalize always flushes bypassing the rate limit, overflow returns false and removes the stale draft, unchanged-text suppression, best-effort throughout. 12 new tests with injected clock/transport. Plan: 100
Replaces drop_pending_updates with a SQLite-backed monotonic offset store. On first startup (no stored offset), probes offset:-1 to discover the current frontier without dispatching anything, preventing an upgrade flood. Replaces bot.start() with a custom poller using grammY's public API (api.getUpdates + bot.handleUpdate) because bot.start() has no initial-offset option. The commit seam is at final delivery, not after await next(): formerly detached text/photo/document/voice orchestration paths now await completion before advancing the offset. At-least-once + dedupe is the honest guarantee, not exactly-once. 5 new tests. Plan: devlog/_plan/260812_windows_and_channels_parity/080
Replaces the existing error handler that shut Discord down for the whole session with a LifecycleSupervisor tracking starting/ready/recovering/ blocked/stopped states per generation. Built against the REAL discord.js 14.26.2 event surface: - shardDisconnect(closeEvent, shardId) β terminal only - shardReconnecting(shardId) β recoverable, no close code - shardResume/shardReady/shardError for lifecycle tracking Close-code classification (1006/4000-series) is NOT observable through discord.js's public API; the supervisor works with what IS observable. Generation fencing prevents stale events from corrupting a newer generation's state. Bounded retry with backoff for recoverable closures. 12 new tests, 90 existing Discord tests green. Plan: 091
Adds a scheduler that queues requests per major-parameter route, enforces a global 429 gate, performs bounded exponential retry with abort support, and uses a body factory for fresh multipart bodies on retry. When two route lanes discover the same bucket, the canonical-lane union merges their queues, fences in-flight requests, and conservatively merges rate-limit state β closing the serialization gap the audit identified. Uses the wp7 DeliveryFailure taxonomy (discordDeliveryError) for error classification. send-only-client.ts now routes through the scheduler. 16 new tests, 168 existing Discord tests green. Plan: 090
Three source-regex tests broke because wp9 replaced bot.start() with TelegramDurablePoller: - grammy-409-defense: bot.start().catch β poller.start().catch - telegram-zombie TZ-005: deleteWebhook before bot.start β deleteWebhook before getUpdates in bootstrapInner - telegram-queue-routing TQ-008: data.requestId (dot notation after readSource normalization, !== negation form) The behavioral guarantees are unchanged; only the code shape moved.
|
Important Review skippedToo many files! This PR contains 101 files, which is 1 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. βοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: π Files selected for processing (101)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Closing β will force-push dev onto main to fix unrelated-histories divergence, then CI deploys via OIDC. |
![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: d8a17d197c
βΉοΈ 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".
Comment on lines
+197
to
+199
| while (!signal.aborted) await this.pollOnce(signal); | ||
| } catch (error) { | ||
| if (!signal.aborted) throw error; |
There was a problem hiding this comment.
When processing or final delivery of any update throwsβfor example, during a transient Telegram API outageβpollOnce() rejects and this outer catch terminates run(). The caller in src/telegram/bot.ts schedules a restart only for HTTP 409, so every other temporary failure leaves Telegram polling permanently stopped until the service is manually restarted, while the uncommitted update remains queued. Retry the same durable offset with bounded backoff instead of ending the polling lifecycle.
Useful? React with πΒ / π.
Comment on lines
+331
to
+334
| if (settings["slack"]?.conversationContext === false) return ''; | ||
| if (!channel) return ''; | ||
| const token = getSlackSendClient().token; | ||
| if (!token) return ''; |
There was a problem hiding this comment.
When slack.senderIdentity is disabled but conversation context remains at its new default, this path still builds a block that can contain human names from a thread identity cache or an enabled channel roster, and the thread preamble can map authors to cached names as well. That violates the setting's existing contract that disabling sender identity prevents human names from reaching prompts; gate or redact all identity-derived context when the opt-out is active.
Useful? React with πΒ / π.
Comment on lines
+203
to
+212
| const tick = () => { | ||
| if (gen !== cliStatusGenRef.current) return; | ||
| const plan = planCliStatusPoll({ | ||
| snapshot: cliStatusRef.current, | ||
| cli: draft.cli, | ||
| attempts, | ||
| now: Date.now(), | ||
| deadline, | ||
| }); | ||
| if (plan.kind === 'stop') return; |
There was a problem hiding this comment.
When /api/settings populates draft.cli before the concurrent initial /api/cli-status request finishes, cliStatusRef.current is still empty here, so the plan returns stop. The later status response may say checking, but cliStatus is intentionally excluded from the effect dependencies, meaning no timer is ever armed and the original permanent βμν νμΈ μ€β state remains. The initial status load must trigger the first polling decision without resetting the bounded polling window.
Useful? React with πΒ / π.
Comment on lines
+68
to
+69
| if ((name === 'python' || name === 'python3') && isWindowsStoreAlias(candidate.path)) { | ||
| return { |
There was a problem hiding this comment.
On Windows systems where where.exe python lists the Microsoft Store alias before a real Python installation later on PATH, this immediate return records Python as unavailable and never examines the valid candidate. Ignore the alias and continue scanning, retaining its diagnostic only if no spawnable Python candidate is ultimately found.
Useful? React with πΒ / π.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What ships
Security
fs.realpathSync.nativeresolves the ADS form, so the check must precede canonicalizationProcess lifecycle
.kill()callsites with idempotent tree termination, fixing two escalation machines that guarded on unreliableChildProcess.killedChannel infrastructure
Windows
detectWindowsShell()with pwshβPowerShellβGitBashβcmd precedenceTelegram
drop_pending_updates: trueoffset:-1prevents upgrade floodDiscord
Evidence