fix(dsh-plugin): preserve the canonical Obelisk workflow - #1
Closed
KinomotoMio wants to merge 27 commits into
Closed
fix(dsh-plugin): preserve the canonical Obelisk workflow#1KinomotoMio wants to merge 27 commits into
KinomotoMio wants to merge 27 commits into
Conversation
…0103#67) BSD mktemp treats only a trailing X run as the unique part. Agents copy the first-pass recipe and add .mjs, which plants /tmp/obq.XXXXXX.mjs and makes every later --query fail before Obelisk runs. Closes tommy0103#66
…3#68) Follow-up to tommy0103#67: instead of only warning about the BSD mktemp trailing-X trap next to the snippet, change the snippet so the trap cannot trigger — each query gets a unique mktemp -d directory and the .mjs file lives inside it, so the template always ends on the X run. Agents that 'improve' the command now get a harmless odd path at worst, and there is no stray empty base file left in /tmp. - SKILL.md: snippet becomes a two-line mktemp -d form; the 5-line warning from tommy0103#67 shrinks to one sentence. - api-reference.md: same snippet update. - pitfalls.md recovery entry stays: hand-rolled mktemp variants can still hit the literal-path trap. - Compatibility preserved: the no-mktemp fallback (mkdir + shell builtins) still produces a unique directory — verified with mktemp removed from PATH, and the BSD path verified on macOS.
Adds ~/.codex/archived_sessions as a Codex transcript root: discovery (full and changed-path indexing), watch roots, and raw record resolution including child-thread lookups. Changed-paths mode now trusts watcher-reported paths over the mtime cursor, aligning codex with the pi/kimi providers. Full-scan mtime guard unchanged.
* Fix Kimi undo replay for injections * fix(core): drop prompt-owned injections when replaying Kimi undo Kimi undo removed the messages inside the undone range but left behind an injection owned by the undone prompt when it sits immediately before that user prompt (e.g. an image_compression injection). After the undo count is satisfied, keep walking back through the contiguous injections whose origin.ownerPromptId matches the last-removed prompt id and drop them too. To support the match, the per-wire undo state now records each real user message's prompt id and each injection's ownerPromptId keyed by projected message uuid. This also retires the previously write-only injectionMessageUuids set. Bump the canonical transcript marker to v6 so existing Kimi sessions re-project under the corrected undo semantics. --------- Co-authored-by: yhuikzdtguioaert <174601769+yhuikzdtguioaert@users.noreply.github.com>
Co-authored-by: yhuikzdtguioaert <174601769+yhuikzdtguioaert@users.noreply.github.com>
…overy and reconcile loop (tommy0103#83) * chore: scaffold @parcel/watcher migration (draft PR) * feat(app): replace chokidar with @parcel/watcher, add dead-stream recovery and reconcile loop * fix(app): keep the OBELISK_DIR watch retrying while its root is unwatched * refactor(app): probe watcher roots with async fs.promises.access, notify on establishment * feat(app): classify watcher probe failures; warn once per root and failure * fix(app): true trailing debounce for OBELISK_DIR changes, async dir ensure, retry repetition test * test(app): model the real onRootLost-driven retry chain in the OBELISK_DIR retry test
…ile poller (ADR-0009, part 1) (tommy0103#88) * chore: scaffold @parcel/watcher migration (draft PR) * docs: ADR-0009 adaptive watcher package + watcher backend research * feat(watcher): adaptive-watcher package, typed watch targets, exact-file poller (ADR-0009, part 1) * fix(watcher): no silent file baseline, close awaits in-flight subscribes, cross-platform build, CI step * fix(watcher): close also awaits unsubscribes of error-dropped subscriptions
… seeding (ADR-0009, part 2) (tommy0103#90) * chore: scaffold hot-transcript overlay (draft PR) * feat(watcher): bounded hot-transcript polling overlay with watchHints seeding (ADR-0009, part 2) * fix(watcher): app indexer watchHints, hint promotion semantics, directory-key guard, cap race * fix(watcher): expand directory hint keys, release missing hot slots, seed after manual rebuild * fix(watcher): mtime-ranked directory expansion, reconcile fallback when rebuild yields no hints * test(watcher): probe-first real-watcher tests for CI FSEvents latency; app-main-settings in CI * test(watcher): mirror USERPROFILE in HOME-mocking tests for Windows CI; late noise re-check * test(app): close the main-process db before temp-home cleanup (Windows EBUSY); safe env restore
…tommy0103#102) * chore: scaffold Claude torn-tail cursor fix (draft PR) * fix(core): Claude cursor must not count a torn unterminated tail line * fix(core): enrich Claude cursor with size/ctime/inode so same-millisecond changes are rediscovered
…ait (tommy0103#86) (tommy0103#103) * chore: scaffold bounded index scheduling (draft PR) * feat(app): bounded index build scheduling with typed batches and maxWait (tommy0103#86) * fix(app): raise nonce poll cap to 8s, delay-aware scheduler tests, end-to-end benchmark * test(app): delay-proven scheduler tests, sustained-convergence case, side-effect-free benchmark * fix(app): revert out-of-scope nonce cap change, A/B trigger attribution in benchmark * test(app): renderer-visible latency probe via CDP (p50 1281ms p95 1799ms) * test(app): renderer probe with concurrent polling and ceiling cadence proof * test(app): complete-cohort renderer probe with drain window and resource cleanup * test(app): probe hygiene — close cursorDb/ws on all paths, track writer-stop timeout
tommy0103#110) * fix(core): semantic read-only SQL validation for sql() (tommy0103#107) Replace the lexical keyword scan in sql() — which false-rejected read-only queries containing blocked words in literals, comments, or quoted identifiers — with validation that follows the statement's actual database effects: - Keep the top-level SELECT/WITH prefix contract (also keeps statement-level PRAGMA out; pragma table-valued functions inside SELECT still work). - Install a prepare-time denylist authorizer on drivers exposing setAuthorizer (node:sqlite): DML, DDL, ATTACH/DETACH, and SAVEPOINT return SQLITE_DENY; everything else — including recursive CTEs and FTS shadow table reads — is allowed by default. The authorizer is the semantic classifier; the read-only connection from openReadDb() remains the final mutation boundary. On better-sqlite3 (no authorizer), the statement readonly flag covers classification instead. - Detect multi-statement input via node:sqlite's sourceSQL, which exposes only the compiled statement: any tail beyond whitespace/comments fails clearly. better-sqlite3 rejects multi-statement input at prepare time. Benchmark (scripts/bench-sql-readonly.mjs, checked in and repeatable): workload median +0.54-0.62 us/call and +5.9-6.2% relative, within the agreed tommy0103#107 budget (<=1 us, <=10%); per-shape table documents the boundary shapes (aggregate ~14% at ~0.36 us, FTS join ~10% at ~1.0 us). Closes tommy0103#107 * test(core): version-robust read-only tests, CI coverage, fairer benchmark Address review on tommy0103#110: - Gate the prepare-time write-rejection assertions on the setAuthorizer capability (Node 24.10+); on Node 22.13-24.9 writes are blocked by the read-only connection instead. Add a file-backed test proving that final boundary fails closed on every supported Node version. - Add tests/query.test.mjs to the CI workflow; the explicit test list did not include it, so the read-only contract tests never ran in CI. - Flip benchmark block order every round so systematic machine drift actually cancels; the header previously claimed alternation the loop did not perform. * bench(core): gate relative budget on workload-aggregate ratio A per-shape relative gate is not repeatable: on shapes with a ~2 us baseline, +-100 ns of timer noise amplifies into +-10% swings and identical code flickers across the 10% line (observed 17.7%/7.3%/7.9% on one commit). Gate the relative budget on the aggregate ratio (sum of per-shape median deltas over sum of legacy medians), which averages that noise over the whole workload; keep the absolute gate on the workload median. Per-shape rows remain printed. Three isolated runs: +548/+681/+598 ns, +2.7/+2.8/+2.3% — stable PASS. * docs(adr): semantic read-only validation for the query sandbox Record the tommy0103#107 decision: layered enforcement (SELECT/WITH prefix contract, prepare-time denylist authorizer where the driver exposes it, sourceSQL multi-statement tail check, read-only connection as the final boundary that never fails open), the per-runtime degraded modes, the deliberately reserved better-sqlite3 readonly capability seam (kept by maintainer decision after review), and the operationalization of the issue's performance budget as workload-median absolute + workload-aggregate relative after per-shape relative medians proved non-repeatable.
…011) (tommy0103#113) * feat(deepseek): add DeepSeek Harness provider — root-tree units with two-path parse (ADR-0011) Adds a deepseek provider adapter indexing DeepSeek Harness session artifacts ($DSH_HOME/sessions, default ~/.dsh/sessions) into the shared schema. Architecture (ADR-0011, supersedes the never-merged file-level design of PR tommy0103#74): - One IndexUnit = one root session TREE (root + descendant subagent files, grouped by project-scoped ancestry), making every cross-file ownership question internal to the unit. - The cursor is a checkpoint: persist-compatible mtime:count prefix plus opaque base64url state — per-member { agentId, headerHash, inode, count, lastEntryHash }, per-member lastMessageUuid, canonical anchor steps. - Fast path (strict preconditions: same member set/identities/inodes, non-decreasing counts, prefix-continuity hash) decodes only new frames/lines and emits countMode 'delta'. Snapshot fallback retracts the checkpointed identity via delete-session and re-parses the whole tree with countMode 'total' — safe because the unit re-emits the entire tree. - Event identity is deterministic from content: dbId = deepseek:<id>: <sha256(cwd)> (composite, pi-style), messages keyed by {turn,step}; provisional tool_use anchors converge to the canonical row via the upstream checkpoint ordering (assistant/message before durable tool/call). - Subagent total_tokens derive at query time per ADR-0010 (provider-stored wins, null-fill otherwise). - Vendored dsh zstd scanner/decoders (read-only port, MIT, attribution kept). Packed chunk rows are not expanded on the indexing path. Tests: real sanitized dsh artifacts (structure-preserving: envelope, framing, event types, numeric shape verbatim; payloads replaced), the split-point equivalence invariant (two-phase incremental ≡ full parse at EVERY frame boundary), snapshot fallback (truncation/replacement/identity change), tombstones, round-trip conformance, and the plaintext/zstd equivalence check. * fix(deepseek): round-4 review — tree-safe reconciliation, identity-keyed tombstones, fail-closed snapshots, full prefix proof, anchor checkpoint, fixture hygiene - changedPaths reconciliation routes by tree, not member path: a deleted child's changed-path no longer matches any member, so it now falls back to the tree owning that project directory and the stale sidechain rows retract instead of surviving forever. - Tombstones key on session IDENTITY, not path: a moved tree produces a new unit with the same session id, and a path-keyed tombstone would delete the fresh snapshot; tombstones now fire only when no discovered unit claims the indexed id. - Fail-closed member reads: a member that exists in discovery but fails to snapshot (corrupt/unreadable) makes parse return the old cursor with zero records — never a partial tree, never a fallback retraction of last-good data (ADR-0001). Unreadable-but-present files stay attached to their project directory's tree (and are reported via reportIncompleteInventory) instead of vanishing from the group. - Prefix continuity now hashes ALL committed frames/lines, not just the boundary entry: an in-place edit of an early frame plus an append no longer reads as pure growth. - The cursor checkpoints every emitted anchor step (canonical AND provisional), so a second durable tool/call of an aborted step in a later window no longer rewrites the provisional anchor's parent/timestamp. - ADR-0010 derivation: a legitimate 0 stays 0; null means 'no usage-bearing messages'. - Fixtures: second-pass global sanitization of user-identifying absolute paths (111 occurrences), with a guard test asserting no user paths remain in any decoded frame. Tests: deleted-child routing, tree move survival, fail-closed member read, early-frame tamper, provisional anchor stability, headerless inventory reporting, fixture path-leak guard. * fix(deepseek): round-5 review — duplicate-identity dedupe, unconditional fallback retraction, project-level fail-closed, generation guard, parent-cycle fix, version gate - Duplicate scoped identity (e.g. a copied session file): dedupe members by identity at grouping; two files of one identity no longer double-emit uuids / double-count aggregates (ADR-0007), and the anomaly is reported. - Fallback always retracts the unit's identity before re-emitting, including the prior-less case — a moved tree (new path, no cursor) that was also truncated no longer keeps stale rows. - Unreadable-but-present files suppress ALL trees of their project dir for the round instead of being attached by a unique-tree guess (which broke multi-tree projects into partial snapshots). - Tombstones only fire when the inventory is complete (source root online, no unreadable files) — an offline root no longer deletes last-good data. - A group whose root file is gone emits no live unit (no child-only phantom snapshot); the identity-keyed tombstone retracts it instead. - TOCTOU: parse re-verifies every member's header hash against the discovery-time identity and bails (keeping last-good) when a replacement landed between the two reads. - Parent-cycle fix: the cursor checkpoints the seed message's own parent, so a step straddling the boundary links its first window message to the seed's parent — no text↔tool_use two-node cycle, and the chain now matches the full parse exactly (asserted). - Header version gate: unknown versions are skipped and recorded, never parsed as v0 (CONTRIBUTING). - ADR-0011 wording aligned with the implementation (prefixHash; the real assistant/message→tool/call ordering). - Tests now use production-shaped key-addressed cursor lookups, multi-tree projects, and both-phase state. * fix(deepseek): round-6 review — complete inventory gating, pre-filter liveness, divergent-copy fail-closed, atomic member snapshots, versioned cursor, honest cycle test - inventoryComplete now covers every reported inventory problem (project readdir errors, unreadable files, divergent duplicates), not just the unreadable list — a partially enumerated inventory can never fire destructive tombstones. - liveSessionIds is computed from all groups with a root member BEFORE the changed-path filter: a cross-directory move reported as only the old path no longer tombstones the moved session. - Divergent copies of one identity fail closed (tree suppressed + recorded) instead of an arbitrary lexicographic winner overwriting last-good; byte-identical copies still dedupe quietly. - snapshotMember pins one file generation: stat+content read under a single fd, header parsed from that same buffer; the version field joins the identity hash so a v0→v99 replacement cannot pass the generation guard. - The cursor checkpoint carries an explicit schema version with defensive defaults; an unrecognized or older shape decodes as no-prior and falls back to a full snapshot (self-healing, no migration). - The parent-cycle regression test now covers the actual scenario it claims: provisional anchor in window 1, canonical assistant/message in window 2 — asserting finite chains, full-parse-identical linkage, and that the canonical row wins (model/usage survive). - ADR-0011: remaining last-entry-hash wording aligned with the cumulative prefixHash implementation. * fix(deepseek): round-7 review — permission loss is not deletion, unroutable-change reconcile, single-fd zstd path - The blocker: existsSync conflates EACCES with ENOENT, so a session dir that temporarily lost permissions looked 'gone' and could be tombstoned. Existence is now probed with statSync and only ENOENT/ENOTDIR count as 'gone'; anything else is an inventory error — the inventory stays uncertified, the tree is suppressed, and no tombstone can fire. - A watcher report that routes to no current group (e.g. only the OLD path of a moved tree) now falls back to reconciling every tree, so provenance (sessions.jsonl_path) and raw() update immediately instead of waiting for the next full reconcile. - The zstd snapshot path no longer re-reads the file by pathname after pinning a generation via fd — one generation throughout. - Comment/ADR drift swept (prefixHash wording everywhere). - The parent-cycle test is honestly labeled as synthetic-order robustness; real-order coverage comes from the split-point equivalence test. Tests: permission-denied member (EACCES) preserves last-good with an inventory issue; old-path-only move report updates provenance immediately. * fix(deepseek): round-8 review — real single-generation zstd snapshot, identity-aware move routing, directory events - The zstd snapshot branch actually re-read the file by pathname after pinning a generation via fd (two identical code blocks kept diverging under targeted edits) — now uses the fd-pinned buffer throughout. - Move routing no longer relies on the project-prefix fallback alone: a changed path that is an indexed session's recorded jsonl_path but no current tree's root (old path of a move whose old project dir still hosts other trees) is unroutable and forces a full reconcile, so the moved tree's provenance updates immediately instead of refreshing an untouched sibling. - Directory-level watcher events (project/session renames reported as the directory path) now trigger reconciliation instead of being dropped by the session-file suffix filter. - Removed the unused innerCtx leftover; comment/doc drift swept. * fix(deepseek): round-9 review — watcher events actually reach the provider; visible root-access errors - The P1 end-to-end breakpoint: the app caller's domain filter only accepted .jsonl/.json, so .jsonl.zstd transcript events and bare directory-rename events were dropped before Core — all changed-path reconciliation was unreachable in production. Per ADR-0009 the domain filter belongs to the caller: indexer-service now forwards .jsonl.zstd and directory-shaped paths, and promotes .jsonl.zstd into the hot set. Covered by a new integration test that mocks the watcher package and drives real invalidations through the caller's wiring. - A sessions root that is present-but-inaccessible (EACCES/EIO) now always reports an inventory issue — including on first index with no history — instead of silently showing empty results; 'gone' keeps its indexed-sessions-only reporting. - ADR-0011: residual 'seeding.' fragment and the provisional/canonical ordering wording corrected. * fix(deepseek): round-10 review — filesystem-based directory detection, root-scoped change routing, first-index visibility - Directory events: the basename-has-a-dot heuristic failed on Windows separators and dotted directory names (repo.v2, and even the default .dsh root). Detection is now filesystem-based: transcript suffix, or an actual directory, or a missing path (the old side of a rename must be forwarded). The transcript suffix list is a single shared constant instead of two copies. - changedPaths are scoped to this provider's root before any routing: foreign providers' writes no longer become unroutable changes that rescan/re-hash every DSH tree. - ADR-0011 anchor-ordering paragraph now states the real semantics (canonical-first in real logs; provisional only for never-completed steps, kept stable by the checkpoint). Tests: first-index inaccessible root reports an inventory issue (zero history); foreign-provider paths trigger no deepseek reconciliation; dotted-directory and missing-rename-source events forward correctly. * fix(deepseek): round-11 review — no sync IO in main process, realpath-aware routing, CI coverage - Directory-event detection no longer stats synchronously on the watcher callback (CONTRIBUTING: no synchronous IO in the main process — a network mount would freeze the UI). Non-transcript paths resolve via fs.promises: real directories and missing paths (rename sources) forward, real stray files are still dropped. Existing noise-filter behavior is preserved (app-indexer-watcher test green). - Watcher paths are matched after realpath canonicalization on BOTH sides: a symlinked $DSH_HOME (or macOS /var → /private/var) no longer makes real DSH events look foreign. - The new deepseek-tree and watcher-filter tests are now part of the CI workflow's explicit test list. - Test hygiene: unified writeFileSync imports. * fix(deepseek): round-12 review — single changed-path routing table The routing logic had accreted into layered patches (prefix filter, canon calls, two maps with overwrite collisions, identity checks, project-dir fallback) that produced the multi-tree move/delete misroutes. It is now one table built once per discovery: - every changed path is canonicalized once (realpath when it exists); - routing precedence is exact member file → session dir → project dir, and every level maps to a SET of trees (no Map<projectDir, groupIndex> overwrite — a child delete in a shared project dir no longer rebuilds or skips the wrong sibling); - checkpointed member paths keep deleted members routing precisely; - indexed sessions' jsonl_paths route a moved tree's old path by identity; - directory events and unroutable paths reconcile all trees. Also: Windows compatibility (permission tests skip on win32 where chmod is a no-op; fixture paths use fileURLToPath instead of URL.pathname) and ADR-0011 documents the routing table. * test(deepseek): derive expected identities from a discovery probe (Windows-safe) The hardcoded scope hash assumed POSIX path normalization; on Windows node:path normalizes the fixture cwd to backslash form, so every identity-based assertion failed. The expected ids now come from a real discover() probe, which follows whatever the provider computes on the host. * fix(deepseek): round-13 review — alias-safe canonicalization, non-circular identity proof, verification contract - canon() resolves through the longest EXISTING ancestor, so the link-spelled and realpath-spelled forms of a deleted/renamed path still converge: a symlinked root + move reported only by old realpath now routes to the moved tree (repro'd from review before/after). - The identity assertions are no longer circular with discover(): the composite shape is asserted explicitly and a different cwd is proven to produce a different scope, so deleting cwd namespacing would fail. - ADR-0011 routing paragraph matches the implementation (two path-keyed maps with set values, bounded ancestor walk). * docs(adr): 0011 routing wording — bounded canonicalization + ancestor lookup
…Harness ADR-0009 (Mode 0): one read-only obelisk_query tool plus a guidance section, mounting the cross-tool archive and memory layer beside the built-in session history search. The tool runs bounded JavaScript through the obelisk CLI sandbox, caps output, and stays a thin translation layer; the obelisk skill keeps owning query semantics. - packages/dsh-plugin: cordis plugin (tool + system prompt section), schemastery config, sample overlay patch, package README - tests/dsh-plugin.test.mjs + fake CLI fixture: registration, JSON round-trip, failure/stderr surfacing, non-JSON passthrough, output cap, spawn errors
Presentation-only frontend contribution per ADR-0009: the package now declares dsh.client (platform web) and ships a lib/client.js bundle that registers a keyed toolview for the obelisk_query wire name — monolith glyph, Obelisk label, bounded query summary, expandable QUERY/RESULT sections on DSW theme tokens. Invariants: the model-facing surface and the durable session record are unchanged; only the human-facing card differs, and only the obelisk_query key is claimed. Skill-driven bash invocations keep the standard row (documented; conditional takeover needs a future generic capability in the harness). - src/client: row-model (pure derivation), obelisk-row card, slot registration, CSS module - tsdown.config.ts: self-contained client bundle build (closure-factory contract + inline CSS modules, adapted from deepseek-harness MIT preset) - tests/dsh-plugin-client-model.test.mjs: 6 model tests - README: frontend-presentation section and rc.6 alignment fix
Supplementary decision: the plugin carries its own browser half via the dsh.client dual-face mechanism and styles first-party obelisk_query calls. Three invariants are recorded — model-facing surface unchanged, durable session record unchanged, only the obelisk_query key claimed — and the bash-invocation recognition is deferred behind a future generic keyed-select capability in the harness.
KinomotoMio
force-pushed
the
fix/dsh-plugin-canonical-skill
branch
from
August 27, 2026 05:06
eb3f09c to
c67640b
Compare
Owner
Author
|
Closed in favor of the upstream merge PR: tommy0103#115 |
![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: c67640bf4e
ℹ️ 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
+49
to
+52
| return Number(parts[0]) !== stat.mtimeMs | ||
| || Number(parts[2]) !== stat.size | ||
| || Number(parts[3]) !== stat.ctimeMs | ||
| || Number(parts[4]) !== stat.ino; |
There was a problem hiding this comment.
When an indexed Claude JSONL is atomically replaced, truncated, or rewritten in place, this signature correctly rediscovers it because the inode, size, or ctime changed, but parse() still derives skip from the old cursor and persists the result as a delta. Consequently, the replacement's first skip lines are ignored while rows from the previous file remain, potentially combining two transcript generations in the index. Distinguish append/tail completion from replacement signatures and force a snapshot/retraction for the latter.
Useful? React with 👍 / 👎.
Comment on lines
+91
to
+95
| for (const entry of entries) { | ||
| if (candidates.length >= HINT_DIRECTORY_CANDIDATE_LIMIT) return; | ||
| const full = join(current, entry.name); | ||
| if (entry.isDirectory()) walk(full, depth + 1); | ||
| else if (entry.name.endsWith('.jsonl')) candidates.push(full); |
There was a problem hiding this comment.
For a Kimi session with at least 16 agent wire files whose directories precede agents/main in readdir order, this early cap stops the DFS before the main wire is collected, so the subsequent mtime ranking cannot select the actively appended transcript. On macOS, if that main wire was already open when the watcher started, it receives neither a native event nor hot polling and can remain stale until the five-minute reconcile. Ensure the main wire is considered or rank all bounded session entries before truncating.
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
7 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.
This PR proposes a focused revision to upstream PR #61. Its base is that PR's source branch,
KinomotoMio:feat/dsh-plugin, so merging this PR updates the upstream contribution directly.Summary
obelisk_querywrapper, prompt fragment, and browser renderer with a bundled provider for the canonicalobeliskskill@obelisk/dsh-obelisk-plugin, which DSH presents asobelisk-pluginDiscussion
The earlier implementation was written before the Obelisk session adapter for DSH existed. It came from a constrained prompt-driven exercise intended to demonstrate three declared DSH plugin aspects without first inspecting DSH source: register a tool, append model guidance, and add a branded browser card. The generated implementation was mechanically coherent with those prompts, but the visual goal ended up deciding the model protocol. The canonical skill taught Bash while the plugin introduced a second invocation path with its own temporary-file handling, synchronous subprocess, timeout, truncation, and error semantics.
During review we separated the cross-agent contract from DSH-specific presentation. The dedicated tool had mainly become necessary because a tool name could own a distinctive frontend renderer. DSH currently cannot let an external plugin decorate only selected Bash calls, and identifying Obelisk inside arbitrary shell pipelines would not provide a reliable attribution boundary anyway. We therefore chose consistency over branding: this revision accepts the ordinary Bash presentation instead of changing DSH, copying its Bash renderer, or keeping a second tool identity.
That distinction required product judgment beyond satisfying the original implementation checklist. Obelisk is meant to help people recover the decisions behind prior work; this integration should demonstrate the same discipline by preserving one agent-facing contract rather than allowing an attractive adapter mechanism to become architecture.
The settled decisions are:
skilltool handle discovery and loadingImplementation
@obelisk/dsh-obelisk-pluginnow depends only on DSH's standardskillsservice. It registers one bundled provider, reads name and description from the canonicalSKILL.md, and exposes the copied skill directory as its resource base. The previous tool schema, system-prompt section, CLI wrapper, configuration surface, client bundle, and frontend tests are removed.The build copies
skill-doc/directly intodist/skill/. Tests compare the loaded body with the canonical source, verify every referenced document exists, and exercise registration and disposal against the real DSH skill registry.ADR-0009 now follows the repository's established
Context/Decision/Verification/Consequencesstructure. It records the durable integration contract in English; implementation-review history remains in this PR discussion rather than in the future architecture record.Verification
npm test— 466 tests passednpm run typecheck --workspace @obelisk/dsh-obelisk-pluginnode --experimental-test-module-mocks --test tests/dsh-plugin.test.mjsnpm exec eslint -- packages/dsh-plugin/src/index.ts tests/dsh-plugin.test.mjsdiff -qr skill-doc packages/dsh-plugin/dist/skillnpm pack --workspace @obelisk/dsh-obelisk-plugin --dry-run --json— the package containsSKILL.mdand every referenced resourceb150a551b8d465e31e418e1b2eaf5e79bbb7d28e; its repository remained cleanobelisk-pluginas mounted and enabledsession-3e4b3515-fbc8-43cf-9d2c-a75eaa1d29af: the model loadedobeliskthroughskill, read bundled references, and issued standard Bashobelisk --querycalls; noobelisk_querycall appeared