feat(sdk): add external agent skill bundle - #4164
Conversation
|
Late full |
![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: 59c1586b8f
ℹ️ 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".
| ) | ||
| digest = hashlib.sha256(payload.encode("utf-8")).hexdigest()[:16] | ||
| challenge = f"APPROVE {session_id} {operation} {digest} {secrets.token_hex(8)}" | ||
| answer = input(f"Approval required: {challenge}\\nType the exact challenge: ") |
There was a problem hiding this comment.
In Python control mode, input(prompt) writes the approval challenge to stdout before the JSON result is printed. External controllers that treat this template as a JSON-producing direct client therefore receive mixed prose and JSON on every successful control request and cannot parse the response reliably; emit the challenge on stderr and read stdin without an stdout prompt, as the TypeScript template does.
Useful? React with 👍 / 👎.
| const ALLOWED_GLOBALS = ["session.create", "session.fork", "session.resume", "session.close"] as const; | ||
|
|
||
| function discoverSkill(): string { | ||
| return `--- |
There was a problem hiding this comment.
The generator makes TypeScript template literals the authoritative source for all three skill prompts. This directly conflicts with the repository contract requiring prompt content to live in static .md files rather than being constructed inline; keep the prose in static source files and have the generator copy or validate those sources.
AGENTS.md reference: AGENTS.md:L120-L120
Useful? React with 👍 / 👎.
| problems.push(`invalid: sdk-skills/${rel}`); | ||
| continue; | ||
| } | ||
| actual = fs.readFileSync(target, "utf8"); |
There was a problem hiding this comment.
The new generator performs its drift checks and generation with synchronous Node filesystem calls such as readFileSync, with corresponding synchronous writes and directory operations later in the same script. The repository contract requires Bun.file()/Bun.write() for file contents and node:fs/promises for directory operations, so this release/CI path should use those APIs instead.
AGENTS.md reference: AGENTS.md:L124-L132
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
NEEDS-WORK — the four-skill gate is clean, but this ships a permanent public on-disk format with no version, and the generator violates the repo's prompt-authoring contract.
The gate is clean — leading with it since the title is alarming
"add external agent skill bundle" sounds like it adds defaults. It does not:
- Nothing under
packages/coding-agent/src/defaults/gjc/skills/orpackages/coding-agent/src/prompts/agents/is touched. The four default workflow skills and four role agents are untouched. - No gate script modified —
check-visible-definitions.ts,verify-g002-gates.ts,rebrand-inventory.ts,default-gjc-definitions.test.tsall unchanged. The PR does not widen its own gate. - No repo-visible
.gjcdefault definitions committed. - Missing, drifted, symlinked or unexpected generated files fail the repository check.
Consistent with the precedent from #4116: skills outside the bundled defaults directory are not covered by the four-skill assertion.
Rebases cleanly. Focused TypeScript 8 pass / 0 fail, focused Python 6 pass / 0 fail.
Finding 1 — the on-disk bundle format is unversioned
docs/sdk.md:79-89 and scripts/generate-gjc-sdk-skills.test.ts:123-135 document and pin exact directory names, a five-file layout, frontmatter, and template paths. Those are permanent public contracts the moment someone authors against them — but there is no manifest and no format version, and no versioned root.
Consumers cannot distinguish a compatible bundle from a future incompatible layout, and there is no way to fail closed on an unsupported version. Adding a version field costs almost nothing now and is close to impossible to retrofit once third parties ship bundles. This is the finding I would most want fixed before merge.
Finding 2 — skill prompts are authored as inline TypeScript strings
scripts/generate-gjc-sdk-skills.ts:23-150 builds SKILL.md bodies as template literals inside functions like discoverSkill().
AGENTS.md is explicit: "Prompts live in static .md files imported with with { type: \"text\" }; never build prompts inline." Same rule, same reason — prompt text should be reviewable and diffable as content, not embedded in a generator.
Make the static Markdown files authoritative and have the generator copy and validate them.
Finding 3 — document the trust boundary plainly
To be fair to the PR, it already discloses the blast radius accurately, and the templates are correctly described as procedural controls rather than a security boundary (sdk-skills/gjc-sdk-operate/SKILL.md:8).
Worth stating explicitly in docs/sdk.md anyway, because it is easy to misread: authentication checks only possession of the endpoint token (packages/coding-agent/src/sdk/host/websocket-transport.ts:151-155). Past that, requests reach the SDK operation registry dispatcher, so a modified script with a valid token bypasses the template allowlist entirely and can reach managed bash, configuration, permissions, tools, extensions and destructive session operations (packages/coding-agent/src/sdk/protocol/operation-registry.ts:66-120).
A reader who sees a curated "skill bundle" may assume the bundle constrains what an external agent can do. It does not — the token does.
Smaller
The Python control test challenges stderr and reads stdin without a stdout prompt, matching the TypeScript implementation, but there is no subprocess-level assertion that a successful control's stdout parses directly as JSON. Worth adding — that is the actual contract an external consumer depends on.
Net
No defect in shipped behaviour and the gate is clear. Version the bundle format and move the prompts to static .md, and this is an approve.
…from static markdown Addresses the maintainer REQUEST_CHANGES on #4164: the on-disk bundle format had no version root, and the generator authored skill prompts inline as TypeScript template literals. - sdk-skills/manifest.json is the versioned root of the bundle contract (formatVersion 1) and declares the exact file closure; checks fail closed on missing, malformed, or unsupported versions, and legacy unversioned layouts are rejected with a regeneration hint instead of being read ambiguously. - The three skill prompts are authored as static markdown under scripts/gjc-sdk-skills/prompts/, imported with `with { type: "text" }`, and copied verbatim by the generator; a consistency validator keeps the static allowlist blocks in sync with the ALLOWED_CONTROLS/ALLOWED_GLOBALS the templates embed. - docs/sdk.md documents the bundle format version contract and the token-only trust boundary (a modified script holding the endpoint token reaches the operation registry dispatcher, not just the template allowlist). - The Python template emits the approval challenge on stderr and reads the answer from stdin, so a successful control's stdout parses directly as JSON; a real-session subprocess test asserts that exact contract. - Tests now prove deterministic closure, four-skill default preservation, no extra skills, upgrade compatibility (v1 installs valid; legacy/future layouts fail closed), and byte-for-byte generated artifact parity. Lore-id: pr-4164-repair Constraint: do not weaken the four-skill gate or existing installed bundles Rejected: inline prompt strings in the generator | violates prompt-authoring contract Rejected: unversioned five-file layout | consumers cannot fail closed on incompatible bundles Confidence: high Scope-risk: narrow Reversibility: revertible Tested: generator suite 14 pass; python template 7 pass incl real-session; check:sdk-skills 28 gates; check:tools; check:public-sync; check-visible-definitions; rebrand-inventory --strict; default-gjc-definitions 29 pass; verify-g002-gates at baseline parity Not-tested: full workspace check:ts aggregate and CI matrix Supersedes: 59c1586 review findings
59c1586 to
fba0187
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba0187647
ℹ️ 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".
| snapshot: dict[str, Any] = {} | ||
| for query in CORE_QUERIES: | ||
| try: | ||
| response = await client.query(query, {}) |
There was a problem hiding this comment.
When a live WebSocket continues answering pings but never returns a query response, this await blocks indefinitely because the Python SdkClient and WsTransport.receive_text() impose no request timeout. The inspection template then neither preserves the remaining partial results nor reaches its finally close; the control await has the same problem after a mutation may already have been accepted. Wrap both query and control requests in a bounded asyncio.wait_for(...) (treating timed-out queries as unavailable) so the direct client terminates predictably.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
The stale review’s substantive requests are addressed: the bundle now has a fail-closed versioned manifest, the prompts are static imported Markdown, the trust boundary is explicit, and the Python subprocess test covers JSON-only stdout. I also reviewed the newest 7bf8e1f0 CI fix; marking root-test:release and release-publish-contract as native consumers matches the commands they execute, and the added assertions cover the emitted matrix contract.
Findings
Major — generator violates the repository filesystem contract
scripts/generate-gjc-sdk-skills.ts:92-97, :480-485, :505-519, and :534-539 implement the new generator/checker with node:fs synchronous APIs (lstatSync, readFileSync, readdirSync, rmSync, mkdirSync, writeFileSync). AGENTS.md explicitly requires Bun.file()/Bun.write() for file I/O and node:fs/promises for directory operations. This is the shipped generator and CI drift gate, not incidental legacy code. Convert these paths to async Bun/promises APIs and make the exported checker/generator entry points asynchronous rather than introducing a new synchronous filesystem surface.
Minor — new test uses forbidden ReturnType<>
scripts/generate-gjc-sdk-skills.test.ts:20 declares Array<ReturnType<typeof Bun.serve>>. AGENTS.md says never to use ReturnType<>; name the concrete Bun server type instead.
The new tests otherwise exercise observable behavior (manifest rejection, drift and symlink rejection, discovery ambiguity, redaction, exact approval binding/replay, endpoint revalidation, and subprocess stdout purity). I found no new placeholder tests, bare not.toThrow(), or mock.module() in the added coverage.
gajae.pr-review-verdict.v1: needs-human
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Terminal Verdict — OWNER_CONFIRMATION_REQUIRED
PR #4164 · feat(sdk): add external agent skill bundle · exact head 7bf8e1f042e6944f7b8d307092a007d8fc5fcf5a (base 7858b0ff63, current dev)
PR-scoped release/root gates — GREEN (exact-head CI)
root-test:release— success (was the only PR-local failure on the prior head; fixed by provisioning the native addon for the release-contract shard viataskNeedsNative, commit7bf8e1f042)root-check— success · Python 3.10–3.13 matrix — success ·gjc-state-gates(static/integrity/read/runtime) — success ·Telegram daemon generation guard— success ·Public site sync— success- Local gates on the same head:
check:sdk-skills28 gates,check:tools,check:public-sync,check-visible-definitions,rebrand-inventory --strict,default-gjc-definitions29 pass, strict mypy over the generated Python template,test:release114 pass, generator suite 14 pass, Python template suite 7 pass incl. real-session subprocess JSON-stdout contract.
The three remaining failures — baseline-reproducible, NOT caused by #4164
Reproduced identically on a clean origin/dev worktree by running the exact failing test files:
shard-1-of-8:session title source persistence > moveTo header patch persistence > rejects when the moved session cwd patch cannot be writtenshard-6-of-8:ultragoal resident-cache adversarial QA > C1 … / C7 …shard-7-of-8:resident cache root derivation(4 cases)
These are the known #4151 sidecar-collision family tracked by #4183 (not yet merged to dev); origin/dev is still 7858b0ff63. Affected path validation and evidence producer fail purely as the downstream aggregate of those shards.
Reviewer findings — all addressed
Versioned on-disk bundle/storage format (sdk-skills/manifest.json, formatVersion: 1, fail-closed on missing/unsupported versions, legacy-layout rejection with migration hint) · skill prompts authored as canonical static Markdown (scripts/gjc-sdk-skills/prompts/, imported with { type: "text" }, copied verbatim, allowlist consistency validator) · trust boundary documented in docs/sdk.md · Python approval challenge moved to stderr with a subprocess-level pure-JSON-stdout assertion. Four-skill gate untouched; deterministic closure, upgrade compatibility, no-extra-skills, and byte-for-byte artifact parity all covered by tests.
Why not MERGE_READY
Exact-head CI is not zero-failure (3 baseline shards + downstream aggregate). The owner must decide: wait for #4183 to land and rerun, or accept the documented baseline-red shards. No merge or release was performed.
Commit history on feat/sdk-skills: 54a368622e (PR reconstructed onto dev) → fba0187647 (review fixes) → 7bf8e1f042 (native-addon provisioning for release-contract shards). Remote lease verified before each force-with-lease push; remote branch confirmed at 7bf8e1f042.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
…from static markdown Addresses the maintainer REQUEST_CHANGES on #4164: the on-disk bundle format had no version root, and the generator authored skill prompts inline as TypeScript template literals. - sdk-skills/manifest.json is the versioned root of the bundle contract (formatVersion 1) and declares the exact file closure; checks fail closed on missing, malformed, or unsupported versions, and legacy unversioned layouts are rejected with a regeneration hint instead of being read ambiguously. - The three skill prompts are authored as static markdown under scripts/gjc-sdk-skills/prompts/, imported with `with { type: "text" }`, and copied verbatim by the generator; a consistency validator keeps the static allowlist blocks in sync with the ALLOWED_CONTROLS/ALLOWED_GLOBALS the templates embed. - docs/sdk.md documents the bundle format version contract and the token-only trust boundary (a modified script holding the endpoint token reaches the operation registry dispatcher, not just the template allowlist). - The Python template emits the approval challenge on stderr and reads the answer from stdin, so a successful control's stdout parses directly as JSON; a real-session subprocess test asserts that exact contract. - Tests now prove deterministic closure, four-skill default preservation, no extra skills, upgrade compatibility (v1 installs valid; legacy/future layouts fail closed), and byte-for-byte generated artifact parity. Lore-id: pr-4164-repair Constraint: do not weaken the four-skill gate or existing installed bundles Rejected: inline prompt strings in the generator | violates prompt-authoring contract Rejected: unversioned five-file layout | consumers cannot fail closed on incompatible bundles Confidence: high Scope-risk: narrow Reversibility: revertible Tested: generator suite 14 pass; python template 7 pass incl real-session; check:sdk-skills 28 gates; check:tools; check:public-sync; check-visible-definitions; rebrand-inventory --strict; default-gjc-definitions 29 pass; verify-g002-gates at baseline parity Not-tested: full workspace check:ts aggregate and CI matrix Supersedes: 59c1586 review findings
7bf8e1f to
349aca6
Compare
|
Repaired and rebased Exact head: Local exact-head evidence: |
|
Updated exact head: In addition to the async SDK bundle repair, this head repairs the dirty current-dev integration discovered in exact-head CI: browser runtime diagnostics had changed the eager tool schema without regenerating Current local evidence: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de1052aa6
ℹ️ 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".
| @@ -0,0 +1,427 @@ | |||
| import { afterEach, describe, expect, it } from "bun:test"; | |||
| import * as fs from "node:fs"; | |||
There was a problem hiding this comment.
The newly added test suite imports node:fs and repeatedly uses rmSync, mkdtempSync, mkdirSync, writeFileSync, and readFileSync for setup, assertions, and cleanup. Repository tooling requires Bun.file()/Bun.write() for file contents and node:fs/promises for directory operations, so convert these helpers and hooks to async equivalents rather than adding a large synchronous filesystem path to the release suite.
AGENTS.md reference: AGENTS.md:L124-L132
Useful? React with 👍 / 👎.
…from static markdown Addresses the maintainer REQUEST_CHANGES on #4164: the on-disk bundle format had no version root, and the generator authored skill prompts inline as TypeScript template literals. - sdk-skills/manifest.json is the versioned root of the bundle contract (formatVersion 1) and declares the exact file closure; checks fail closed on missing, malformed, or unsupported versions, and legacy unversioned layouts are rejected with a regeneration hint instead of being read ambiguously. - The three skill prompts are authored as static markdown under scripts/gjc-sdk-skills/prompts/, imported with `with { type: "text" }`, and copied verbatim by the generator; a consistency validator keeps the static allowlist blocks in sync with the ALLOWED_CONTROLS/ALLOWED_GLOBALS the templates embed. - docs/sdk.md documents the bundle format version contract and the token-only trust boundary (a modified script holding the endpoint token reaches the operation registry dispatcher, not just the template allowlist). - The Python template emits the approval challenge on stderr and reads the answer from stdin, so a successful control's stdout parses directly as JSON; a real-session subprocess test asserts that exact contract. - Tests now prove deterministic closure, four-skill default preservation, no extra skills, upgrade compatibility (v1 installs valid; legacy/future layouts fail closed), and byte-for-byte generated artifact parity. Lore-id: pr-4164-repair Constraint: do not weaken the four-skill gate or existing installed bundles Rejected: inline prompt strings in the generator | violates prompt-authoring contract Rejected: unversioned five-file layout | consumers cannot fail closed on incompatible bundles Confidence: high Scope-risk: narrow Reversibility: revertible Tested: generator suite 14 pass; python template 7 pass incl real-session; check:sdk-skills 28 gates; check:tools; check:public-sync; check-visible-definitions; rebrand-inventory --strict; default-gjc-definitions 29 pass; verify-g002-gates at baseline parity Not-tested: full workspace check:ts aggregate and CI matrix Supersedes: 59c1586 review findings
0de1052 to
f287bc5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a17c9390
ℹ️ 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".
| #!/usr/bin/env bun | ||
|
|
||
| import { createHash, randomBytes } from "node:crypto"; | ||
| import { lstat, readFile, readdir } from "node:fs/promises"; |
There was a problem hiding this comment.
The remaining generated TypeScript template imports the Node filesystem helpers directly and later reads endpoint records with readFile, while the repository contract requires Bun.file() for file contents and namespace imports from node:fs/promises for directory operations. Update both this generated artifact and its authoritative generator so regeneration does not restore the prohibited implementation.
AGENTS.md reference: AGENTS.md:L124-L132
Useful? React with 👍 / 👎.
| import { lstat, readFile, readdir } from "node:fs/promises"; | ||
| import * as path from "node:path"; | ||
| import { createInterface } from "node:readline/promises"; | ||
| import { SdkClient, listSdkSessionEndpoints, type SdkSessionEndpoint } from "@gajae-code/coding-agent/sdk"; |
There was a problem hiding this comment.
When an external user follows docs/sdk.md and installs only the recommended @gajae-code/bridge-client, this canonical template immediately fails because it instead resolves @gajae-code/coding-agent/sdk; a globally installed gjc package does not satisfy local module resolution. Either use the standalone client and provide discovery locally, or explicitly make installation of @gajae-code/coding-agent part of the bundle's setup contract and test the template outside the monorepo workspace.
Useful? React with 👍 / 👎.
| [path.join("gjc-sdk-discover", "SKILL.md"), discoverPrompt], | ||
| [path.join("gjc-sdk-operate", "SKILL.md"), operatePrompt], | ||
| [path.join("gjc-sdk-author", "SKILL.md"), authorPrompt], | ||
| [path.join("gjc-sdk-author", "templates", "direct-sdk.ts"), typeScriptTemplate()], | ||
| [path.join("gjc-sdk-author", "templates", "direct-sdk.py"), pythonTemplate()], |
There was a problem hiding this comment.
On Windows, these path.join calls produce backslash-separated logical bundle keys, so manifestFile() renders a different file list from the committed forward-slash contract and bun run check:sdk-skills reports drift; running generation there also rewrites the versioned manifest incompatibly. Use fixed POSIX-relative keys for the map and reserve path.join for converting those keys to filesystem paths.
Useful? React with 👍 / 👎.
| const discoveryDirectory = path.join(repo, ".gjc", "state", "sdk"); | ||
| const directoryStat = await lstat(discoveryDirectory).catch(() => undefined); | ||
| if (!directoryStat || directoryStat.isSymbolicLink() || !directoryStat.isDirectory()) |
There was a problem hiding this comment.
When <repo>/.gjc or <repo>/.gjc/state is a symlink, lstat() on the final sdk directory follows that intermediate link and reports an ordinary directory, allowing the template to select and authenticate to out-of-repository discovery records despite its documented fail-closed handling for symlinked discovery. Validate every path component or compare a resolved path against the intended repository root; the Python template's final-component-only check has the same gap.
Useful? React with 👍 / 👎.
|
Updated exact head: The async SDK bundle repair now has a cross-platform fail-closed read boundary: POSIX reads use Exact-head local evidence: |
|
@probepark The prior filesystem-contract findings are repaired on exact head |
|
Exact-head CI run I reproduced and repaired every actionable code blocker locally, and re-ran failed jobs for the exact pushed head |
…from static markdown Addresses the maintainer REQUEST_CHANGES on #4164: the on-disk bundle format had no version root, and the generator authored skill prompts inline as TypeScript template literals. - sdk-skills/manifest.json is the versioned root of the bundle contract (formatVersion 1) and declares the exact file closure; checks fail closed on missing, malformed, or unsupported versions, and legacy unversioned layouts are rejected with a regeneration hint instead of being read ambiguously. - The three skill prompts are authored as static markdown under scripts/gjc-sdk-skills/prompts/, imported with `with { type: "text" }`, and copied verbatim by the generator; a consistency validator keeps the static allowlist blocks in sync with the ALLOWED_CONTROLS/ALLOWED_GLOBALS the templates embed. - docs/sdk.md documents the bundle format version contract and the token-only trust boundary (a modified script holding the endpoint token reaches the operation registry dispatcher, not just the template allowlist). - The Python template emits the approval challenge on stderr and reads the answer from stdin, so a successful control's stdout parses directly as JSON; a real-session subprocess test asserts that exact contract. - Tests now prove deterministic closure, four-skill default preservation, no extra skills, upgrade compatibility (v1 installs valid; legacy/future layouts fail closed), and byte-for-byte generated artifact parity. Lore-id: pr-4164-repair Constraint: do not weaken the four-skill gate or existing installed bundles Rejected: inline prompt strings in the generator | violates prompt-authoring contract Rejected: unversioned five-file layout | consumers cannot fail closed on incompatible bundles Confidence: high Scope-risk: narrow Reversibility: revertible Tested: generator suite 14 pass; python template 7 pass incl real-session; check:sdk-skills 28 gates; check:tools; check:public-sync; check-visible-definitions; rebrand-inventory --strict; default-gjc-definitions 29 pass; verify-g002-gates at baseline parity Not-tested: full workspace check:ts aggregate and CI matrix Supersedes: 59c1586 review findings
b7a17c9 to
8c1fccd
Compare
|
Current-dev integration refresh completed. Exact head: The rebase preserved the SDK bundle repairs and both Unreleased entries. Post-rebase checks passed: |
|
@probepark Please re-review exact refreshed head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1fccd8f6
ℹ️ 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".
| const result = | ||
| args.mode === "inspect" | ||
| ? await inspect(client, endpoint.token) | ||
| : await client.control(args.operation!, args.input, { confirm: true }); |
There was a problem hiding this comment.
When turn.prompt is accepted but its response is lost or exceeds SdkClient's default timeout, this call rejects and the template reports only a generic failure, even though the prompt may already be executing. Because turn.prompt is ordered and non-idempotent, rerunning the template after another approval can create a duplicate agent turn; require a fresh caller-retained clientRef for prompts and reconcile turn.prompt_status after an uncertain send rather than leaving retry safety to guesswork.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
APPROVE — every finding from my two earlier rounds is fixed, exact-head CI is clean, and the one blocker I cited last round (baseline-red shards) no longer exists. One non-blocking scope note and two nits below.
Reviewed head: 8c1fccd8f6e954902f787f476941a9ddc7a1fa4f (21 files, +2140/−7).
Previously requested changes — all verified fixed
| Round | Finding | Status |
|---|---|---|
| #1 | On-disk bundle format unversioned | Fixed. sdk-skills/manifest.json with bundle/formatVersion: 1/files; validateBundleManifest rejects missing, malformed, wrong-bundle, non-integer, unsupported-version, and file-list-mismatch cases, each with a regeneration hint. Version check runs before content drift checks, so an unknown layout is never content-diffed. |
| #1 | Prompts embedded in generator (violates prompt-authoring contract) | Fixed. scripts/gjc-sdk-skills/prompts/{author,discover,operate}.md imported with { type: "text" } and copied verbatim; validatePromptAllowlistConsistency() proves the rendered allowlist blocks in operate.md still match ALLOWED_CONTROLS/ALLOWED_GLOBALS. |
| #1 | Trust boundary not stated in docs | Fixed. docs/sdk.md "Trust boundary" says it plainly: auth checks only token possession, no per-request capability check, dispatcher reaches managed bash/config/permissions/tools/extensions, and the allowlists are "not a security boundary". |
| #1 | Python control test lacked stdout-JSON assertion | Fixed. python/gjc-sdk/tests/test_generated_skill_template.py asserts subprocess stdout parses as pure JSON. |
| #2 (Major) | Generator used sync node:fs against AGENTS.md |
Fixed. node:fs/promises + Bun.file()/Bun.write() throughout; node:fs is imported only for Stats/constants. checkSdkSkillFiles, readBundleManifest, validateInstalledBundle, writeFiles are all async now. |
| #2 (Minor) | ReturnType<> in test |
Fixed. const servers: Bun.Server<undefined>[]. |
I re-swept every .ts/.py file this PR adds for ReturnType<>, mock.module(), bare not.toThrow(), await import(), readFileSync/writeFileSync, and : any — zero hits in the new files. (The remaining hits in the sweep are pre-existing lines in scripts/ci-dev-affected.ts/.test.ts, which this PR only touches by +2 lines each.)
Last round's blocker is gone
I withheld MERGE_READY last time solely because exact-head CI had 3 baseline-red shards from the #4151/#4183 sidecar-collision family. That is resolved:
#4183is merged intodev(b8f6c2645), andgit merge-base --is-ancestor origin/dev pr-4164→ true, so this branch already contains it.- Checks at
8c1fccd8f6: 46 SUCCESS / 5 SKIPPED / 0 FAILURE, 2 cargo-build jobs still in progress. No red shards.
Gate re-verification (unchanged and clean)
- No file under
packages/coding-agent/src/defaults/gjc/skills/orsrc/prompts/agents/touched — the four workflow skills and four role agents are untouched. - No gate script modified (
check-visible-definitions.ts,verify-g002-gates.ts,rebrand-inventory.ts,default-gjc-definitions.test.ts). The PR does not widen its own gate. - No repo-visible
.gjc/default definitions committed. check:sdk-skillsis wired into bothcheck:sdk-closureandci:check:full, so drift is enforced rather than advisory.tsconfig.tools.jsontypechecks the standalone TS template;check:py-sdknow runs strict mypy over the Python template too../sdkis a real published export (packages/coding-agent/package.json→./src/sdk/index.ts, andsrcis infiles), andgjc_sdkexportsEndpoint/SdkClient/read_session_endpoint/select_live_endpoint— both templates resolve for an external consumer, not just in-repo.
Findings
Minor — the tool-catalog regeneration is unrelated scope, and it is repairing a live dev defect
packages/coding-agent/src/tools/tool-catalog.generated.ts (+5/−1, commit ae8956bc4 fix(tools): regenerate browser diagnostics catalog) has nothing to do with an SDK skill bundle. It matters more than a stray file, because dev is genuinely stale right now:
origin/dev:packages/coding-agent/src/tools/browser.ts:90declaresdiagnostics: z…(landed inb70775c44).origin/dev:…/tool-catalog.generated.tshas no"diagnostics": {under the browser entry — its only"diagnostics"at line 1012 belongs to a different tool's enum.- This PR adds it back at line 1188.
So b70775c44 shipped a browser parameter without regenerating the catalog, and the repair is currently trapped behind a 2100-line feature PR. Cherry-pick ae8956bc4 to its own PR so dev gets the generated surface back in sync immediately; this PR then carries only the bundle. Not blocking the approval — I'd rather this land than sit.
Nit — readRegularFile conflates "absent" with "unreadable"
scripts/generate-gjc-sdk-skills.ts:109-127 returns null from a bare catch, and checkSdkSkillFiles maps every null to missing: sdk-skills/<rel> + "Run bun run generate-sdk-skills". An EACCES, EIO, or ELOOP therefore reports as a missing file and sends the operator to a regeneration that will not fix it. The verification logic is right to fail closed; only the diagnostic is lossy. Distinguishing ENOENT from everything else (and naming the code in the other branch) would keep the same fail-closed behaviour with an honest message.
Nit — the identity tuple is weaker than it reads on Windows
isSameRegularFileIdentity includes mtimeMs, which is attacker-controllable via utimes and has coarse resolution on some filesystems. On POSIX this is belt-and-braces on top of O_NOFOLLOW + fd-stat, so it is fine; on Windows it is the only check, since the O_NOFOLLOW term is 0 there. Worth a comment stating that the Windows path is best-effort, so nobody later mistakes it for a symlink-proof guarantee.
Notes, not requests
writeFiles()doesfs.rm(bundleDir, { recursive: true, force: true })before rendering. Correct for a fully-owned generated directory, and the file-set self-test covers stale-file removal — just flagging that it makessdk-skills/strictly generator-owned, which the docs now say explicitly.- Both templates are symmetric on input validation (
--modeenum-checked,--inputrejects non-objects/arrays), approval bindssha256({sessionId, operation, input})plus a fresh nonce,workflow.gate_answeris forced to carryexpectedSessionId, and the endpoint is re-selected and compared field-by-field (includingtoken) after approval. That is the right ordering; I could not construct a replay or endpoint-swap against it.
Verdict
gajae.pr-review-verdict.v1 merge-approved sha256:8c1fccd8f6e954902f787f476941a9ddc7a1fa4f reviewer:human evidence:https://github.com/Yeachan-Heo/gajae-code/pull/4164/checks
Reviewer is probepark, author is Yeachan-Heo — distinct accounts. If those are the same operator in practice, repo policy makes this self-approval and the verdict must be downgraded to needs-human before merge; every technical criterion I can check is otherwise satisfied. Merge after the 2 in-flight cargo jobs go green.
MERGE_READYExact head The repaired branch contains the asynchronous, fail-closed cross-platform SDK bundle reader; async test fixture conversion; deterministic manifest/content symlink coverage; and the regenerated browser diagnostics tool catalog. Local exact-head verification is recorded in the preceding signed comments and Ultragoal ledger. No merge or release was performed from this lane. |
External coding agents need a host-neutral, direct-SDK contract for discovering and safely operating local GJC sessions without MCP or coordinator coupling. The generated bundle owns exactly five files, enforces deterministic drift checks, and ships credential-safe TypeScript and Python templates with fail-closed discovery and nonce-bound approval. Lore-id: 7f1a3bd1 Constraint: generated bundle must contain exactly five gjc-sdk-* artifacts Constraint: endpoint bearer tokens must never be rendered or persisted Rejected: MCP or coordinator integration | superseded by direct SDK-only scope Confidence: high Scope-risk: medium Reversibility: clean-revert Tested: generated verifier 25 gates; TypeScript 8 tests; Python 36 tests; strict mypy; release 108 tests; focused SDK 10 tests
…from static markdown Addresses the maintainer REQUEST_CHANGES on #4164: the on-disk bundle format had no version root, and the generator authored skill prompts inline as TypeScript template literals. - sdk-skills/manifest.json is the versioned root of the bundle contract (formatVersion 1) and declares the exact file closure; checks fail closed on missing, malformed, or unsupported versions, and legacy unversioned layouts are rejected with a regeneration hint instead of being read ambiguously. - The three skill prompts are authored as static markdown under scripts/gjc-sdk-skills/prompts/, imported with `with { type: "text" }`, and copied verbatim by the generator; a consistency validator keeps the static allowlist blocks in sync with the ALLOWED_CONTROLS/ALLOWED_GLOBALS the templates embed. - docs/sdk.md documents the bundle format version contract and the token-only trust boundary (a modified script holding the endpoint token reaches the operation registry dispatcher, not just the template allowlist). - The Python template emits the approval challenge on stderr and reads the answer from stdin, so a successful control's stdout parses directly as JSON; a real-session subprocess test asserts that exact contract. - Tests now prove deterministic closure, four-skill default preservation, no extra skills, upgrade compatibility (v1 installs valid; legacy/future layouts fail closed), and byte-for-byte generated artifact parity. Lore-id: pr-4164-repair Constraint: do not weaken the four-skill gate or existing installed bundles Rejected: inline prompt strings in the generator | violates prompt-authoring contract Rejected: unversioned five-file layout | consumers cannot fail closed on incompatible bundles Confidence: high Scope-risk: narrow Reversibility: revertible Tested: generator suite 14 pass; python template 7 pass incl real-session; check:sdk-skills 28 gates; check:tools; check:public-sync; check-visible-definitions; rebrand-inventory --strict; default-gjc-definitions 29 pass; verify-g002-gates at baseline parity Not-tested: full workspace check:ts aggregate and CI matrix Supersedes: 59c1586 review findings
…ards test:release now runs scripts/generate-gjc-sdk-skills.test.ts, whose generated direct-sdk TypeScript recipes spawn the template as a subprocess. The template imports @gajae-code/coding-agent/sdk, whose barrel eagerly loads the @gajae-code/natives addon, so the recipe exits 1 when the shard lacks the prebuilt .node file. taskNeedsNative() covered "root-test" and "test:*" but not "root-test:release" (or the release-publish-contract task that also runs test:release), so those matrix entries shipped native:false and skipped the "Download native addon(s)" step. Add both keys to the native-consumer allowlist; the affected planner already guarantees a native build task exists whenever a native consumer is planned (full-workspace branch and ensureNativeBuild). Lore-id: pr-4164-repair Rejected: skipping the template runtime tests without natives | weakens credential/control contract Confidence: high Scope-risk: narrow Reversibility: revertible Tested: test:release 114 pass; ci-dev-affected.test.ts 87 pass incl new native-flag assertions; dev-ci-guard-topology 20 pass; check:tools Not-tested: exact-head CI rerun Supersedes: fba0187 (root-test:release CI failure)
The generated SDK skill bundle gate used synchronous node filesystem APIs despite the repository contract.\n\nThe generator, drift checker, verifier, and self-test now use Bun and promise-based filesystem operations while preserving fail-closed symlink and manifest validation.\n\nLore-id: 9f8b7d6c\nConstraint: bundle validation must remain fail-closed for missing, drifted, or symlinked files\nConfidence: high\nScope-risk: narrow\nReversibility: straightforward\nTested: bun run check:sdk-skills; bun test scripts/generate-gjc-sdk-skills.test.ts; bun --cwd=packages/coding-agent run check; bun run check:tools; bun run test:py-sdk
The async checker still inspected a path before reading it by name, which let a symlink replacement evade the intended no-follow boundary.\n\nBundle reads now hold O_NOFOLLOW descriptors through validation and read fixture setup uses the repository asynchronous filesystem conventions.\n\nLore-id: 2a4c6e8f\nConstraint: manifest and content reads must reject symlink replacement\nConfidence: high\nScope-risk: narrow\nReversibility: straightforward\nTested: bun test scripts/generate-gjc-sdk-skills.test.ts; bun run check:sdk-skills; bun --cwd=packages/coding-agent run check
Windows does not support O_NOFOLLOW, so descriptor-only validation could follow a replacement symlink.\n\nWindows bundle reads now compare regular-file path and descriptor identities before and after reading while POSIX retains O_NOFOLLOW descriptor reads.\n\nLore-id: 1b3d5f7h\nConstraint: generated bundle reads must reject links on every supported platform\nConfidence: high\nScope-risk: narrow\nReversibility: straightforward\nTested: bun test scripts/generate-gjc-sdk-skills.test.ts; bun run check:sdk-skills; bun --cwd=packages/coding-agent run check; bun run check:tools
Fail-closed bundle validation must not hang when an expected file is replaced by a FIFO.\n\nPOSIX opens now include O_NONBLOCK alongside O_NOFOLLOW, allowing the descriptor regular-file check to reject FIFOs immediately.\n\nLore-id: 4e6f8a0b\nConstraint: bundle checks must reject special files without blocking\nConfidence: high\nScope-risk: narrow\nReversibility: straightforward\nTested: bun test scripts/generate-gjc-sdk-skills.test.ts; bun run check:sdk-skills; bun --cwd=packages/coding-agent run check
8c1fccd to
9f0c7dc
Compare
The bundle checker must reject special files without blocking.\n\nThe Linux/POSIX regression replaces an expected skill file with a FIFO and asserts the checker returns drift failure.\n\nLore-id: 5b7d9f1a\nTested: bun test scripts/generate-gjc-sdk-skills.test.ts; bun run check:sdk-skills; bun --cwd=packages/coding-agent run check
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d888bf90d
ℹ️ 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".
| await fs.rm(bundleDir, { recursive: true, force: true }); | ||
| for (const [rel, content] of files) { | ||
| const target = path.join(bundleDir, rel); | ||
| await fs.mkdir(path.dirname(target), { recursive: true }); |
There was a problem hiding this comment.
Remove this explicit parent-directory creation, as Bun.write() already creates missing ancestors; the same redundant pattern also appears in runSelfTest. Besides adding repeated filesystem work, it conflicts with the repository's mandated Bun filesystem convention.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
| : await client.control(args.operation!, args.input, { confirm: true }); | ||
| process.stdout.write(JSON.stringify(redact({ sessionId: endpoint.sessionId, result }, endpoint.token), null, 2) + "\\n"); | ||
| } finally { | ||
| await client.close(); |
There was a problem hiding this comment.
When the peer returns a successful control response but stalls the WebSocket close handshake, SdkClient.close() rejects from this finally, so the outer catch reports failure and sets a nonzero exit code even though the mutation succeeded and its JSON was already written. Controllers that rely on exit status may discard that result and retry the mutation; treat teardown failure as best-effort once a definitive response has been received.
Useful? React with 👍 / 👎.
|
Exact head |
|
Exact-head CI run This exactly matches the current-dev fixture defect owned by |
2 participants
Summary
gjc-sdk-*external-agent bundleVerification
bun test scripts/generate-gjc-sdk-skills.test.ts(8 pass)bun run check:sdk-skills(25 gates)bun run check:toolsbun run test:release(108 pass)Ultragoal
sha256:8852026a293cfe2c0838de56cc9718a96972d03ea74778d673e7413f169f074b