feat(skills): ship @conciv/skills — first-party agent skills with CI drift lint - #419
feat(skills): ship @conciv/skills — first-party agent skills with CI drift lint#419omridevk wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdded the public ChangesSkills package
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/skills/skills/_artifacts/domain_map.yaml`:
- Around line 1-6: Regenerate the artifacts using the actual generation date of
August 10, 2026: update the Date field in
packages/skills/skills/_artifacts/domain_map.yaml lines 1-6 and the generated_at
field in packages/skills/skills/_artifacts/skill_tree.yaml lines 7-10.
In `@packages/skills/skills/_artifacts/skill_tree.yaml`:
- Around line 56-59: Update the sources list in the skill metadata to include
the authoritative extension definitions
packages/extension/src/define-extension.ts and
packages/extension/src/define-tool.ts, while preserving the existing entries
unless they are specifically identified as stale. Ensure generated metadata
points consumers to these current API contracts.
In `@packages/skills/skills/conciv-debug/SKILL.md`:
- Around line 79-89: Rename the read-only-tool scenario heading to “A read-only
command unexpectedly requires approval” so it accurately reflects the approval
behavior described in the section body, including shell metacharacters forcing
approval.
- Line 29: Update the “No button at all, any framework” troubleshooting row in
the conciv-debug skill to remove the claim that a production build alone
prevents mounting. Document that mounting is prevented only when the call site’s
enabled gate disables conciv, while preserving the existing resolveConfig
default behavior.
- Line 32: Restore the missing third table cell in the troubleshooting row by
adding a Fix column value after the existing Cause cell, matching the table
header and preserving the current symptom and cause text.
In `@packages/skills/skills/conciv-develop/SKILL.md`:
- Around line 15-18: Remove the reference to
packages/harness/plugins/claude/skills/conciv-extensions/SKILL.md from the
conciv-develop skill. Replace it with a self-contained scope boundary or
publicly available documentation, while preserving the distinction between
consumer usage and monorepo contributor work.
In `@packages/skills/skills/conciv-harness/references/transcript-history.md`:
- Around line 94-99: Align both skills with the HarnessHistory API: in
packages/skills/skills/conciv-harness/references/transcript-history.md:94-99,
limit TranscriptFailure claims to methods whose signatures return that union; in
packages/skills/skills/conciv-harness/SKILL.md:274-276, document messages(),
observe(), and list() as required methods.
In `@packages/skills/skills/conciv-harness/SKILL.md`:
- Around line 176-191: Rewrite the “Sandbox-virtual workdir” section to state
that harness workdirs are sandbox-virtual, adapters should default to
/workspace, and adapters must never receive or construct host-absolute cwd
values. Remove the current guidance to pass ctx.cwd or deps.cwd verbatim, and
describe the launch/configuration behavior consistently for both connect.plan()
and chatConfig().
- Around line 117-123: Update the chatConfig adapter guidance to require a
published `@tanstack/ai-`* text adapter for every CLI. Remove the makeTextAdapter
and custom AsyncIterable<StreamChunk> fallback, including instructions to spawn
or decode CLIs through sandbox process handles; retain only the
published-adapter requirement and codexText example.
In `@packages/skills/skills/conciv-setup/SKILL.md`:
- Line 3: Format the description metadata with oxfmt using the configured
120-column print width. Apply this to the description in
packages/skills/skills/conciv-setup/SKILL.md:3-3 and
packages/skills/skills/conciv-develop/SKILL.md:3-3, preserving the text while
wrapping each line as needed.
- Around line 83-85: Update both examples with development-only guards: pass an
explicit development-environment enabled value to the webpack plugin created by
conciv.default(), and wrap the manual widget mount in the host app’s
development-only condition. Keep the existing plugin and mount behavior
unchanged during development.
In `@packages/skills/src/check-references.ts`:
- Line 149: Update citationPattern in check-references.ts to accept all
supported source-file extensions, including .json, and capture complete
comma-separated line-range expressions such as 31-98,100-108 so they are
validated rather than skipped. Add fixtures covering both a .json citation and a
multi-range citation, preserving existing citation forms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d1718419-8894-44fe-81ab-9c0a7452c58d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.changeset/conciv-skills-pack.md.fallowrc.json.github/workflows/ci.ymlpackage.jsonpackages/publish/src/guards.tspackages/skills/package.jsonpackages/skills/skills/_artifacts/domain_map.yamlpackages/skills/skills/_artifacts/skill_spec.mdpackages/skills/skills/_artifacts/skill_tree.yamlpackages/skills/skills/conciv-debug/SKILL.mdpackages/skills/skills/conciv-develop/SKILL.mdpackages/skills/skills/conciv-develop/references/rpc-and-testing.mdpackages/skills/skills/conciv-develop/references/tool-contract.mdpackages/skills/skills/conciv-harness/SKILL.mdpackages/skills/skills/conciv-harness/references/transcript-history.mdpackages/skills/skills/conciv-setup/SKILL.mdpackages/skills/src/check-references.ts
| # domain_map.yaml | ||
| # Generated by skill-domain-discovery | ||
| # Library: @conciv/skills | ||
| # Version: 0.0.18 | ||
| # Date: 2026-08-11 | ||
| # Status: reviewed |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Regenerate the artifacts with a valid generation date.
Both generated files declare August 11, 2026, but the current date is August 10, 2026. Use the actual generation date so artifact provenance remains valid.
packages/skills/skills/_artifacts/domain_map.yaml#L1-L6: update theDatefield during generation.packages/skills/skills/_artifacts/skill_tree.yaml#L7-L10: update thegenerated_atfield during generation.
📍 Affects 2 files
packages/skills/skills/_artifacts/domain_map.yaml#L1-L6(this comment)packages/skills/skills/_artifacts/skill_tree.yaml#L7-L10
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/_artifacts/domain_map.yaml` around lines 1 - 6,
Regenerate the artifacts using the actual generation date of August 10, 2026:
update the Date field in packages/skills/skills/_artifacts/domain_map.yaml lines
1-6 and the generated_at field in
packages/skills/skills/_artifacts/skill_tree.yaml lines 7-10.
| - **A read-only tool that never needed approval is stuck instead.** Approval is decided by | ||
| `requiresApproval`/`needsApproval` (`packages/core/src/chat/gate.ts:56-71`): a tool is risky if its | ||
| name is in the `risky` set, or (for `Bash`) if `classifyCommand` doesn't classify it as `'allow'`. | ||
| The allowlist is intentionally narrow — plain read-only commands (`ls`, `cat`, `grep`, `git status`, | ||
| etc., `READ_ONLY_COMMANDS`/`GIT_READ_ONLY_SUBCOMMANDS`, `packages/core/src/chat/gate.ts:17-34`) plus | ||
| anything the harness adds via `commandAllows`. Any shell metacharacter (`;`, `&`, `|`, backtick, | ||
| `$`, `>`, `<`) forces `'ask'` even if the base command is allowlisted | ||
| (`SHELL_METACHARACTER_PATTERNS`, `packages/core/src/chat/gate.ts:36-49`) — `ls foo | grep bar` asks | ||
| even though both halves are individually allowed. If a command you expected to run silently is | ||
| instead sitting on an approval card, check it for a metacharacter before assuming the allowlist is | ||
| wrong. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rename the read-only-tool scenario.
The heading says the tool “never needed approval”, but the body explains that shell metacharacters force approval. Rename the heading to “A read-only command unexpectedly requires approval” so it matches the documented gate behavior.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 68: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 102: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 111: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 145: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[warning] 138: [EA4] Unbounded Resource Access: Skill allows unbounded resource consumption (API calls, storage, compute). Without rate limits or quotas, a compromised or misbehaving agent can cause denial-of-service or cost overruns.
Remediation: Set explicit rate limits, timeouts, and resource quotas for API calls, file operations, and compute. Implement circuit breakers for runaway loops.
(Excessive Agency (EA4))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-debug/SKILL.md` around lines 79 - 89, Rename
the read-only-tool scenario heading to “A read-only command unexpectedly
requires approval” so it accurately reflects the approval behavior described in
the section body, including shell metacharacters forcing approval.
| (`packages/protocol/src/harness-types.ts:125-140`.) `chatConfig` is a plain function, `deps => | ||
| HarnessChatConfig` — no async, no side effects at call time. Its `adapter` is a published | ||
| `@tanstack/ai-*` text adapter for CLIs that have one (`codexText` from `@tanstack/ai-codex`); if none | ||
| exists for your CLI, build one with `makeTextAdapter(name, stream)` from | ||
| `packages/harness/src/_shared/text-adapter.ts:45-47`, where `stream` is your own | ||
| `AsyncIterable<StreamChunk>` producer that spawns the CLI **through the sandbox process handle**, not | ||
| `node:child_process` directly (see the sandbox-virtual workdir section below). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the custom CLI-spawning fallback.
This section tells consumers to build a custom stream that spawns and decodes the CLI. That creates a second process-lifecycle path and conflicts with the required chat() ownership model. Require a published @tanstack/ai-* text adapter instead.
As per coding guidelines, harnesses must use published @tanstack/ai-* text adapters and must never spawn or decode CLIs directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-harness/SKILL.md` around lines 117 - 123,
Update the chatConfig adapter guidance to require a published `@tanstack/ai-`*
text adapter for every CLI. Remove the makeTextAdapter and custom
AsyncIterable<StreamChunk> fallback, including instructions to spawn or decode
CLIs through sandbox process handles; retain only the published-adapter
requirement and codexText example.
Source: Coding guidelines
| @@ -0,0 +1,226 @@ | |||
| --- | |||
| name: conciv-setup | |||
| description: Use when installing or mounting the conciv dev-agent widget in an app — adding @conciv/it to a Vite/webpack/Rspack/Next.js/Rollup/esbuild build, wiring a manual createConciv/mountConciv setup for a bundler with no plugin, choosing/configuring a harness (claude/codex), or setting the conciv() / ConcivSettingsInit config surface. Covers first install through a working chat button in dev. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Format the skill metadata with oxfmt.
packages/skills/skills/conciv-setup/SKILL.md#L3-L3: wrap the description to the configured 120-column width.packages/skills/skills/conciv-develop/SKILL.md#L3-L3: wrap the description to the configured 120-column width.
As per coding guidelines, **/*.{ts,tsx,js,jsx,json,css,md} must use oxfmt with a 120-column print width.
📍 Affects 2 files
packages/skills/skills/conciv-setup/SKILL.md#L3-L3(this comment)packages/skills/skills/conciv-develop/SKILL.md#L3-L3
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-setup/SKILL.md` at line 3, Format the
description metadata with oxfmt using the configured 120-column print width.
Apply this to the description in
packages/skills/skills/conciv-setup/SKILL.md:3-3 and
packages/skills/skills/conciv-develop/SKILL.md:3-3, preserving the text while
wrapping each line as needed.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
Adds the published @conciv/skills package with first-party agent guidance and CI drift detection.
Changes:
- Adds four setup, development, harness, and debugging skills.
- Adds source-reference validation and Intent checks to CI.
- Registers the package for discovery, publishing, and release.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Registers the package and Intent dependency. |
packages/skills/src/check-references.ts |
Implements citation drift checking. |
packages/skills/skills/conciv-setup/SKILL.md |
Adds setup guidance. |
packages/skills/skills/conciv-harness/SKILL.md |
Adds harness guidance. |
packages/skills/skills/conciv-harness/references/transcript-history.md |
Documents transcript parsing. |
packages/skills/skills/conciv-develop/SKILL.md |
Adds extension-authoring guidance. |
packages/skills/skills/conciv-develop/references/tool-contract.md |
Documents tool contracts. |
packages/skills/skills/conciv-develop/references/rpc-and-testing.md |
Documents RPC and testing. |
packages/skills/skills/conciv-debug/SKILL.md |
Adds runtime debugging guidance. |
packages/skills/skills/_artifacts/skill_tree.yaml |
Records generated skill metadata. |
packages/skills/skills/_artifacts/skill_spec.md |
Defines skill coverage. |
packages/skills/skills/_artifacts/domain_map.yaml |
Maps skills to domains. |
packages/skills/package.json |
Configures the published package. |
packages/publish/src/guards.ts |
Adds the package to the public set. |
package.json |
Enables workspace skill discovery. |
.github/workflows/ci.yml |
Adds skill validation to CI. |
.fallowrc.json |
Marks the package as public API. |
.changeset/conciv-skills-pack.md |
Adds release metadata. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/skills/src/check-references.ts:273
- This TypeScript string uses a prohibited em dash; replace it with repository-compatible punctuation.
return {file: fileRelative, kind: 'dead-citation', detail: `${citation.raw} — ${resolved} does not exist`}
packages/skills/src/check-references.ts:285
- This TypeScript string introduces an em dash, contrary to the repository's code laws.
detail: `${citation.raw} — none of [${[...identifiers].join(', ')}] found in ${resolved}`,
packages/skills/src/check-references.ts:355
- The success message also contains a prohibited em dash.
`skills check:refs: ${skillCount} skill(s), ${checkedFiles} file(s), ${checkedCitations} citation(s) — all clean`,
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function identifiersMatch(paragraph: string, citationPath: string, resolved: string) { | ||
| const excludeTokens = excludeTokensFor(citationPath) | ||
| const identifiers = extractIdentifiers(paragraph, excludeTokens) | ||
| if (identifiers.size === 0) return {ok: true, identifiers} | ||
| const citedContent = readFileSync(join(repoRoot, resolved), 'utf8') | ||
| const ok = [...identifiers].some((identifier) => citedContent.includes(identifier)) | ||
| return {ok, identifiers} | ||
| } | ||
|
|
||
| function checkCitation( | ||
| fileRelative: string, | ||
| paragraph: string, | ||
| citation: Citation, | ||
| basenameMap: Map<string, string>, | ||
| ): SkillFinding | null { | ||
| const resolved = resolveCitationPath(citation.path, basenameMap) | ||
| if (!resolved) return unresolvedFinding(fileRelative, citation) | ||
| if (!existsSync(join(repoRoot, resolved))) return deadCitationFinding(fileRelative, citation, resolved) | ||
| const match = identifiersMatch(paragraph, citation.path, resolved) |
| return { | ||
| file: fileRelative, | ||
| kind: 'unmatchable-citation', | ||
| detail: `${citation.raw} — cannot resolve bare filename "${citation.path}" against this skill's Sources`, |
| The plugin does not add a `<script>` tag. Serve the prebuilt global bundle (published as | ||
| `@conciv/widget/global` per the init tool's own manual card) and point `widgetUrl` at it so the | ||
| script lands on the page (`apps/site/content/docs/quick-start/webpack.mdx:53-57`, | ||
| `packages/cli/src/init/steps/framework/webpack-family.ts:35`): |
| also fully supported. `gemini-cli`, `opencode`, and `pi` exist as ids but are stubs, not implemented | ||
| adapters (`apps/site/content/docs/harnesses.mdx:21-43`, `packages/protocol/src/config-types.ts:35-36` | ||
| for the `harness`/`harnessBin` fields): |
| - A harness id other than `claude`/`codex` picked for real use — `gemini-cli`/`opencode`/`pi` are | ||
| unimplemented stubs. |
| (`packages/protocol/src/harness-types.ts:126`) are therefore already the right value to hand your CLI | ||
| verbatim as its working directory or `--cwd`/`-C` flag. |
| } | ||
| ``` | ||
|
|
||
| `messages()` and `list()` are the required minimum: parse the CLI's own on-disk transcript into |
| inputSchema: z.object({env: z.enum(['staging', 'prod'])}), | ||
| outputSchema: z.object({url: z.string()}), | ||
| promptSnippet: 'You can deploy with the deploy_run tool.', | ||
| meta: {summary: 'deploy the current branch', category: 'deploy', mutating: true}, |
| "lint": "oxlint", | ||
| "intent:validate": "intent validate skills --check", | ||
| "intent:stale": "intent stale --json", | ||
| "check:refs": "node --experimental-strip-types src/check-references.ts" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (20)
packages/skills/src/check-references.ts:294
citation.lineis never used: this reads the entire source and accepts any identifier found anywhere in it. A citation moved beyond EOF or pointing at unrelated lines therefore passes, defeating the advertised line-drift check. Parse and bounds-check the cited range, then match only against that slice.
const citedContent = readFileSync(join(repoRoot, resolved), 'utf8')
const ok = [...identifiers].some((identifier) => citedContent.includes(identifier))
packages/skills/skills/conciv-setup/SKILL.md:95
- This setup cannot work today: no
@conciv/widgetpackage exists,@conciv/embedexports only its mount module, andwidgetUrlis merely resolved inpackages/core/src/config.tswithout any runtime consumer. Shipping this as consumer guidance leads webpack/Rspack users to an unresolvable package and a no-op option. Remove this path until the bundle export and injection mechanism are implemented, or implement both in this PR.
The plugin does not add a `<script>` tag. Serve the prebuilt global bundle (published as
`@conciv/widget/global` per the init tool's own manual card) and point `widgetUrl` at it so the
script lands on the page (`apps/site/content/docs/quick-start/webpack.mdx:53-57`,
`packages/cli/src/init/steps/framework/webpack-family.ts:35`):
```ts
conciv({widgetUrl: '/conciv-widget.js'})
**packages/skills/skills/conciv-setup/SKILL.md:151**
* This is stale against the current adapters: Gemini uses `acpChatConfig` and OpenCode uses `opencodeText` with models, history, and connect support; only Pi still uses `unsupportedChatConfig`. The setup skill would incorrectly steer users away from two working harnesses.
harness on the plugin config (or ConcivConfig passed to createConciv's engine side) picks the
agent CLI behind the chat. claude is the default and needs the claude CLI on PATH; codex is
also fully supported. gemini-cli, opencode, and pi exist as ids but are stubs, not implemented
adapters (apps/site/content/docs/harnesses.mdx:21-43, packages/protocol/src/config-types.ts:35-36
for the harness/harnessBin fields):
**packages/skills/skills/conciv-setup/SKILL.md:203**
* This repeats the stale harness classification: `gemini-cli` and `opencode` have implemented chat adapters. Keeping this red flag would tell agents to reject valid configurations.
- A harness id other than
claude/codexpicked for real use —gemini-cli/opencode/piare
unimplemented stubs.
**packages/skills/skills/conciv-harness/SKILL.md:182**
* This instructs adapter authors to pass the host-absolute project path into a sandboxed CLI, which is the workdir landmine the repository guidance explicitly forbids. The local-process sandbox already maps its root to the host cwd; adapters should use the sandbox-relative root (normally `/workspace`) rather than feeding `deps.cwd` into adapter `cwd`/`--cwd` options, or they can create the nested `Users/...` tree described below.
localProcessSandbox({dir: cwd}) makes the sandbox's process root be deps.cwd — the sandbox
root IS the project cwd, not a subdirectory of it. HarnessConnectContext.cwd
(packages/protocol/src/harness-types.ts:43) and HarnessChatDeps.cwd
(packages/protocol/src/harness-types.ts:126) are therefore already the right value to hand your CLI
verbatim as its working directory or --cwd/-C flag.
**packages/skills/skills/conciv-harness/SKILL.md:276**
* The required minimum is misstated. `HarnessHistory` requires `messages`, `observe`, and `list`; omitting `observe` fails the adapter type. Include it in the minimum so authors do not implement an incomplete sidecar.
messages() and list() are the required minimum: parse the CLI's own on-disk transcript into
UIMessage[] and enumerate past sessions for a cwd. observe() returns a live-tailing
TranscriptHandle (revision()/read()/close()) for streaming updates into an open chat pane — build
**packages/skills/skills/conciv-develop/SKILL.md:58**
* The featured mutating deployment tool omits the approval gate, even though the skill later requires `approval: 'ask'` for state-changing tools. `meta.mutating` does not itself trigger approval (`requiresApproval` checks only the `approval` field), so an agent following this example can deploy without user confirmation.
const deployRun = defineTool({
name: 'deploy_run',
description: 'Deploy the current branch',
inputSchema: z.object({env: z.enum(['staging', 'prod'])}),
outputSchema: z.object({url: z.string()}),
promptSnippet: 'You can deploy with the deploy_run tool.',
meta: {summary: 'deploy the current branch', category: 'deploy', mutating: true},
})
**packages/skills/skills/conciv-develop/SKILL.md:261**
* This standalone augmentation example uses `RegisterExtension` without importing it, so copying it produces `Cannot find name 'RegisterExtension'`. Include `import type {RegisterExtension} from '@conciv/extension'` as shown in the cited implementation.
declare module '@conciv/protocol/config-types' {
interface ExtensionRegistry extends RegisterExtension<typeof terminal> {}
}packages/skills/src/check-references.ts:268
- The repository's code law forbids em dashes in TS/JS string literals. Use a plain separator here.
detail: `${citation.raw} — cannot resolve bare filename "${citation.path}" against this skill's Sources`,
packages/skills/src/check-references.ts:273
- The repository's code law forbids em dashes in TS/JS string literals. Use a plain separator here.
return {file: fileRelative, kind: 'dead-citation', detail: `${citation.raw} — ${resolved} does not exist`}
packages/skills/src/check-references.ts:285
- The repository's code law forbids em dashes in TS/JS string literals. Use a plain separator here.
detail: `${citation.raw} — none of [${[...identifiers].join(', ')}] found in ${resolved}`,
packages/skills/src/check-references.ts:355
- The repository's code law forbids em dashes in TS/JS string literals. Use a plain separator here.
`skills check:refs: ${skillCount} skill(s), ${checkedFiles} file(s), ${checkedCitations} citation(s) — all clean`,
packages/skills/skills/conciv-setup/SKILL.md:172
- This table presents
widgetUrlas a usable webpack/Rspack injection target, but the option has no runtime consumer and the referenced widget package does not exist. Mark this integration unsupported (or remove the row) until the injection path is implemented.
| `widgetUrl` | unset | webpack/Rspack manual injection target |
packages/skills/skills/conciv-setup/SKILL.md:197
- This red flag implies that adding
widgetUrlfixes webpack/Rspack, but the option is currently unused and cannot make a widget appear. Replace this with an explicit unsupported-integration warning so agents do not keep debugging a no-op setting.
- webpack/Rspack setup with the plugin added but no `widgetUrl` — the engine boots but nothing ever
appears on the page.
packages/skills/skills/conciv-debug/SKILL.md:30
widgetUrlcannot currently fix webpack/Rspack: it has no runtime consumer, and the documented@conciv/widget/globalpackage does not exist. Point this scenario to a supported integration rather than a no-op option.
| No button, webpack/Rspack | Plugin boots the engine but never injects a `<script>` tag | set `widgetUrl` and serve the bundle yourself — see conciv-setup's webpack/Rspack section |
packages/skills/src/check-references.ts:149
- The matcher skips the
title="path.ts:N"citation form used throughout these new skills (for example,conciv-develop/SKILL.md:47) and excludes cited JSON files. Those references are therefore never counted or checked, so CI can stay green while they drift. Extend extraction to cover every citation syntax and cited source extension used by the skills.
const citationPattern = /`([A-Za-z0-9_./-]+\.(?:ts|tsx|md|mdx)):(\d+)(?:-(\d+))?`/g
packages/skills/skills/conciv-harness/references/transcript-history.md:96
- This overstates the failure union: only
TranscriptHandle.revision()and.read()returnTranscriptFailure;close()returnsvoid, and the optionalHarnessHistoryrefinements use their individually declared return types (for example,meta/summaryreturnnull). Following this text would produce incompatible method signatures.
(`packages/protocol/src/harness-types.ts:155-161`.) Every `TranscriptHandle` method and `HarnessHistory`'s
optional refinements that can fail return this discriminated union instead of throwing — build one
with `transcriptFailure(reason, detail)` (`packages/harness/src/_shared/jsonl-handle.ts:27-29`). Use
packages/skills/skills/conciv-develop/SKILL.md:47
- The cited range ends before
DeploySurface(the source function starts at line 33 and continues through line 46), although the snippet includes that function. Correct the range; this is also an example the new reference checker currently misses because it does not parse code-fence titles.
This issue also appears on line 258 of the same file.
```tsx title="apps/examples/tanstack-start/conciv/extensions/deploy-button.tsx:1-32"
packages/skills/skills/conciv-debug/SKILL.md:29
- The cause and fix contradict each other: a production build alone does not hide conciv because
enableddefaults totrue; the button is absent only when the integration is actually disabled or not injected. Reword this row so agents do not diagnose an unguarded production widget as an expected production-only absence.
This issue also appears on line 30 of the same file.
| No button at all, any framework | Running a production build | conciv mounts by convention only in dev; check the `enabled` gate at the call site (`resolveConfig` defaults `enabled` to `true`, `packages/core/src/config.ts:27`, so a missing gate ships to prod, it does not hide there) |
packages/skills/skills/conciv-debug/SKILL.md:53
- This fix is not available to consumers through
ConcivConfig:allowedOriginsexists only on the internalStartOpts, while the public plugin config has no such field. For a standard@conciv/itsetup, tunnels/LAN origins cannot simply be added as instructed. State that limitation or expose and plumb a validated public option before recommending it.
- **CORS / wrong origin.** The engine only allows loopback origins (`localhost`, `127.0.0.1`, `[::1]`)
plus anything in `allowedOrigins` by default (`packages/core/src/lib/cors.ts` —
`originAllowed`/`LOOPBACK_HOSTNAMES`). A widget served from a non-loopback host (a tunnel, a LAN IP,
a container hostname) needs that host added to `allowedOrigins` in the `StartOpts` that boot the
engine, or requests fail CORS silently in the browser console before the widget ever renders a chat.
|
Addressed all three review streams (Copilot, CodeRabbit, and an internal 7-hat adversarial panel) in f653029..214db5b: Fixed
Declined with reason
Release note: first-ever publish of All gates green: repo typecheck/format, package test 12/12, typecheck, oxlint, intent validate 4/4, check:refs 143 citations clean, fallow 0 introduced. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/skills/skills/conciv-harness/SKILL.md (1)
266-268: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the required-method count.
HarnessHistoryrequiresmessages(),observe(), andlist(). The preceding text says that only two methods are required. Change that count to three so adapter authors implement the complete contract.Also applies to: 284-287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/skills/skills/conciv-harness/SKILL.md` around lines 266 - 268, Update the transcript-history documentation to state that HarnessHistory requires three methods, messages(), observe(), and list(), while preserving the existing optional-refinement description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/skills/skills/conciv-develop/SKILL.md`:
- Around line 58-60: Update the tool configuration example and accompanying
guidance in the skill document to require approval: 'ask' for mutating tools,
including deployRun. Remove the statement that deployRun can omit approval:
'ask', while preserving the existing mutating metadata and approval behavior
documentation.
In `@packages/skills/skills/conciv-setup/SKILL.md`:
- Around line 122-129: Update both manual mount examples around createConciv and
mountConciv to use host-appropriate development guards: replace
process.env.NODE_ENV with import.meta.env.DEV in the browser/Vite example,
retain process.env.NODE_ENV !== 'production' only for the Next.js example, and
ensure mountConciv is also inside its development guard.
In `@packages/skills/src/check-references.ts`:
- Around line 16-23: Update the source-path validation flow in checkReferences
to resolve and validate each source with withinRepoRoot before calling
existsSync; when the path escapes repoRoot, emit an escaping-source finding and
skip existence checks. Add escaping-source to FindingKind and create a fixture
containing an existing file outside the temporary repository to cover this case.
---
Outside diff comments:
In `@packages/skills/skills/conciv-harness/SKILL.md`:
- Around line 266-268: Update the transcript-history documentation to state that
HarnessHistory requires three methods, messages(), observe(), and list(), while
preserving the existing optional-refinement description.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2caaef8d-3217-42f8-836e-f214801b49b7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/skills/package.jsonpackages/skills/skills/_artifacts/skill_tree.yamlpackages/skills/skills/conciv-debug/SKILL.mdpackages/skills/skills/conciv-develop/SKILL.mdpackages/skills/skills/conciv-develop/references/rpc-and-testing.mdpackages/skills/skills/conciv-develop/references/tool-contract.mdpackages/skills/skills/conciv-harness/SKILL.mdpackages/skills/skills/conciv-harness/references/transcript-history.mdpackages/skills/skills/conciv-setup/SKILL.mdpackages/skills/src/check-references.tspackages/skills/src/cli.tspackages/skills/test/check-references.test.tspackages/skills/tsconfig.jsonpackages/skills/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/skills/skills/_artifacts/skill_tree.yaml
- packages/skills/skills/conciv-harness/references/transcript-history.md
- packages/skills/skills/conciv-develop/references/tool-contract.md
- packages/skills/skills/conciv-develop/references/rpc-and-testing.md
| meta: {summary: 'deploy the current branch', category: 'deploy', mutating: true}, | ||
| approval: 'ask', | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 '\b(mutating|approval|requiresApproval|needsApproval)\b' \
packages/core/src packages/extension/srcRepository: conciv-dev/conciv
Length of output: 43820
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- local skill files ---'
fd -HI 'SKILL\.md$|skill' . | head -80
printf '%s\n' '--- target documentation ---'
cat -n packages/skills/skills/conciv-develop/SKILL.md | sed -n '45,95p'
printf '%s\n' '--- gate and registration paths ---'
cat -n packages/core/src/chat/gate.ts | sed -n '56,123p'
cat -n packages/core/src/app.ts | sed -n '172,191p'
cat -n packages/extension/src/tool-registry.ts | sed -n '312,330p;458,474p'Repository: conciv-dev/conciv
Length of output: 9106
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- TanStack Intent references and workspace instructions ---'
rg -n -i 'TanStack Intent|intent skill|skills check|skill check' \
--glob '!node_modules' --glob '!dist' --glob '!build' . || true
printf '%s\n' '--- approval and mutating call sites ---'
rg -n -C 5 'requiresApproval\(|meta\.mutating|mutating:|approval:' \
packages/core/src packages/extension/src packages/skills/skills/conciv-develop/SKILL.mdRepository: conciv-dev/conciv
Length of output: 24719
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
gate = Path('packages/core/src/chat/gate.ts').read_text()
app = Path('packages/core/src/app.ts').read_text()
registry = Path('packages/extension/src/tool-registry.ts').read_text()
code_mode = Path('packages/core/src/chat/code-mode.ts').read_text()
skill = Path('packages/skills/skills/conciv-develop/SKILL.md').read_text()
def body(text, signature):
start = text.index(signature)
brace = text.index('{', start)
depth = 0
for i in range(brace, len(text)):
if text[i] == '{':
depth += 1
elif text[i] == '}':
depth -= 1
if depth == 0:
return text[brace:i + 1]
raise ValueError(signature)
requires = body(gate, "export function requiresApproval")
registry_backed = body(app, "function registryBackedTool")
compile_tool = body(registry, "function compileTool")
code_call = body(code_mode, "return async (args, context) =>")
print("documentation claims:")
for line_no, line in enumerate(skill.splitlines(), 1):
if 50 <= line_no <= 90 and re.search(r"mutating|approval", line, re.I):
print(f"{line_no}: {line}")
print("\nrequiresApproval body:")
print(requires)
print("\nregistryBackedTool body:")
print(registry_backed)
print("\ncompileTool approval/meta excerpt:")
for line in compile_tool.splitlines():
if re.search(r"approval|mutating|meta|registryBase", line):
print(line)
print("\ncode-mode approval excerpt:")
for line in code_call.splitlines():
if re.search(r"requiresApproval|gate|execute", line):
print(line)
assert "subject.approval === 'ask'" in requires
assert "tool.approval" in registry_backed
assert "tool.meta?.mutating" in registry_backed
assert "requiresApproval(capability)" in code_call
assert "approval" not in requires.split("return", 1)[0] or True
print("\nresult: `meta.mutating` is not an approval condition in the inspected gate path; explicit `approval: 'ask'` is required for this path.")
PYRepository: conciv-dev/conciv
Length of output: 1404
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat -n packages/skills/skills/conciv-develop/SKILL.md | sed -n '50,90p'
printf '\n--- gate decision predicates ---\n'
sed -n '56,72p;115,122p' packages/core/src/chat/gate.ts
printf '\n--- capability construction ---\n'
sed -n '181,190p' packages/core/src/app.ts
printf '\n--- code-mode gate ---\n'
sed -n '67,84p' packages/core/src/chat/code-mode.tsRepository: conciv-dev/conciv
Length of output: 4607
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- code-mode construction and gate wiring ---'
rg -n -C 8 'makeCodeMode|makeRunGate|makeAskGate|PermissionGate|codeMode' \
packages/core/src --glob '*.{ts,tsx}'
printf '%s\n' '--- exact documentation text ---'
cat -n packages/skills/skills/conciv-develop/SKILL.md | sed -n '50,90p'Repository: conciv-dev/conciv
Length of output: 41593
Document approval: 'ask' as required for mutating tools. approvalGatedNames() uses requiresApproval(), which checks only approval: 'ask'. meta.mutating sets the mutating flag but does not enable approval. The text at lines 82–84 teaches an unsafe configuration by saying that the real deployRun omits approval: 'ask'.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 386: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-develop/SKILL.md` around lines 58 - 60, Update
the tool configuration example and accompanying guidance in the skill document
to require approval: 'ask' for mutating tools, including deployRun. Remove the
statement that deployRun can omit approval: 'ask', while preserving the existing
mutating metadata and approval behavior documentation.
| if (process.env.NODE_ENV !== 'production') { | ||
| const handle = createConciv({ | ||
| extensions: [], | ||
| apiBase: 'http://localhost:5178', | ||
| }) | ||
| await handle.mount(document.getElementById('conciv-root')!) | ||
| // later: handle.unmount() | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use a browser-safe development gate for both manual mount examples.
Line 122 uses process.env.NODE_ENV in code that accesses document. Vite does not provide the Node.js process global in browser code, so this example can fail before createConciv runs.
Lines 143-146 call mountConciv without the development gate required by Lines 116-117. Show host-specific browser-safe conditions, such as import.meta.env.DEV for Vite and process.env.NODE_ENV !== 'production' for Next.js. Guard mountConciv too.
#!/bin/bash
set -euo pipefail
rg -n -C 3 'process\.env\.NODE_ENV|import\.meta\.env\.DEV|mountConciv\(' packages appsAlso applies to: 143-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-setup/SKILL.md` around lines 122 - 129, Update
both manual mount examples around createConciv and mountConciv to use
host-appropriate development guards: replace process.env.NODE_ENV with
import.meta.env.DEV in the browser/Vite example, retain process.env.NODE_ENV !==
'production' only for the Next.js example, and ensure mountConciv is also inside
its development guard.
| type FindingKind = | ||
| | 'dead-source' | ||
| | 'dead-citation' | ||
| | 'unmatchable-citation' | ||
| | 'out-of-range-citation' | ||
| | 'empty-glob' | ||
| | 'missing-sources' | ||
| | 'escaping-citation' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'withinRepoRoot|checkSourcesSection|extractSourcesPaths|escaping-citation' \
packages/skills/src/check-references.ts
rg -n -C 4 'escaping|Sources section|dead source' \
packages/skills/test/check-references.test.tsRepository: conciv-dev/conciv
Length of output: 5469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- applicable skill files ---'
find .. -type f \( -iname '*skill*' -o -iname 'AGENTS.md' -o -iname 'CONTRIBUTING.md' \) -print | head -80
printf '%s\n' '--- source implementation ---'
cat -n packages/skills/src/check-references.ts | sed -n '180,240p;300,440p'
printf '%s\n' '--- test helpers and source-path fixtures ---'
cat -n packages/skills/test/check-references.test.ts | sed -n '1,180p'Repository: conciv-dev/conciv
Length of output: 9927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository root and guidance ---'
pwd
for file in AGENTS.md .github/skills/code-review/SKILL.md; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
cat -n "$file" | sed -n '1,220p'
fi
done
printf '%s\n' '--- source implementation ---'
cat -n packages/skills/src/check-references.ts | sed -n '180,240p;300,440p'
printf '%s\n' '--- test helpers and source-path fixtures ---'
cat -n packages/skills/test/check-references.test.ts | sed -n '1,180p'Repository: conciv-dev/conciv
Length of output: 42406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const {existsSync} = require('node:fs')
const {join, resolve, sep} = require('node:path')
const repoRoot = '/home/jailuser/git'
const sourcePath = '../../../etc/hosts'
const joined = join(repoRoot, sourcePath)
const resolvedRoot = resolve(repoRoot)
const absolute = resolve(repoRoot, sourcePath)
const withinRoot = absolute === resolvedRoot || absolute.startsWith(resolvedRoot + sep)
console.log(JSON.stringify({
repoRoot,
sourcePath,
joined,
sourceExists: existsSync(joined),
resolvedCitationPath: absolute,
withinRepoRoot: withinRoot,
}, null, 2))
if (!existsSync(joined) || withinRoot) process.exit(1)
JSRepository: conciv-dev/conciv
Length of output: 349
Reject source paths outside the repository root.
At line 428, existsSync(join(repoRoot, sourcePath)) accepts existing paths such as ../../../etc/hosts, even though the resolved path is outside repoRoot. Call withinRepoRoot before checking existence and emit an escaping-source finding. Add a fixture with an existing file outside the temporary repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/src/check-references.ts` around lines 16 - 23, Update the
source-path validation flow in checkReferences to resolve and validate each
source with withinRepoRoot before calling existsSync; when the path escapes
repoRoot, emit an escaping-source finding and skip existence checks. Add
escaping-source to FindingKind and create a fixture containing an existing file
outside the temporary repository to cover this case.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
packages/skills/skills/conciv-setup/SKILL.md:102
widgetUrlis currently only copied intoResolvedConcivConfig; no plugin or runtime code reads it, and the webpack/Rspack hooks only boot the engine. Consequently this example never injectsconciv-widget.global.js, so users following the skill still get no widget. Either implement the documentedwidgetUrlbehavior or replace this with explicit host-HTML injection (including a matchingpw-api-base/pinned engine port).
conciv({enabled: process.env.NODE_ENV !== 'production', widgetUrl: '/conciv-widget.js'})
packages/skills/skills/conciv-setup/SKILL.md:27
- This blanket production warning is incorrect for two advertised integrations: Vite's hook has
apply: 'serve', while both the Next.js register entry and widget entry explicitly return/skip in production. Only webpack/Rspack currently boot on production builds without anenabledguard. Please scope this guidance, including the later red-flag entry, so agents do not report valid Vite/Next.js setups as production leaks.
This issue also appears on line 102 of the same file.
The second recurring failure is shipping the widget to production: conciv mounts in dev only by
convention, not by a build-time strip — `enabled` defaults to `true`
(`resolveConfig`, `packages/core/src/config.ts:27`), so a plugin call left unguarded stays live in a
production bundle. Every quick-start gates it: `conciv({enabled: process.env.NODE_ENV !==
'production'})` (`apps/site/content/docs/quick-start/vite.mdx:93-95`).
packages/skills/src/check-references.ts:429
- Source paths are joined without the repository-boundary check used for citations. A
## Sourcesentry such as../../some-existing-file.tstherefore passes, even though it resolves outside the repository and cannot ground the skill against this codebase. ReusewithinRepoRootbeforeexistsSyncand add the corresponding source-escape fixture.
return extractSourcesPaths(text)
.filter((sourcePath) => !existsSync(join(repoRoot, sourcePath)))
.map((sourcePath): SkillFinding => ({file: fileRelative, kind: 'dead-source', detail: sourcePath}))
packages/skills/skills/_artifacts/domain_map.yaml:157
- This mechanism reverses the sandbox rule documented by
conciv-harness: omitting a process cwd inchatConfigis correct; passing the host-absolutedeps.cwdinto the sandboxed adapter is what creates the nested tree. Conversely,connect.plan()runs outside that sandbox, so its correct example should continue to usectx.cwd. Keeping the two seams conflated here can regenerate contradictory guidance.
mechanism: 'localProcessSandbox({dir: cwd}) makes the sandbox root BE deps.cwd; a CLI defaulting to /workspace when cwd is not threaded through nests a junk Users/... tree inside the workspace.'
…drift lint New docs-only published package with four tanstack-intent skills (setup, develop, harness, debug) for external consumers' coding agents, plus a check:refs script that fails CI when skill citations drift from source. intent validate blocks repo-checks; intent stale reports non-blocking pre-1.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…apes Line/range citations were never bounds-checked against the target file (file.ts:999999 passed silently) and identifier matching scanned the whole file instead of near the cited line, so a moved API stayed green. Also add comma multi-range parsing, case-insensitive Sources matching, empty-glob detection, and repo-root path containment for citations. Threads repo root through as a parameter (runCheck(rootDir)) with a thin src/cli.ts entry point so the checker is unit-testable, and adds test coverage with real fixture files in temp dirs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Correct the harness cwd guidance (chatConfig never passes host cwd; the sandbox owns it — connect.plan is the host-terminal seam), fix the stale harness support matrix (gemini-cli/opencode are real, only pi stubs), require observe() in the HarnessHistory minimum, add missing enabled gates and approval:'ask' to examples, drop the wrong stale-docs claims, and repair table/citation drift the hardened checker now catches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
214db5b to
2e0ea49
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔇 Additional comments (1)
packages/skills/skills/conciv-harness/references/transcript-history.md (1)
76-87: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.Revalidate project ownership after transcript replacement.
verifyHeadis cached for the handle lifetime. The truncation path resetsoffsetandfold, but it does not resetverified. A new transcript at the same path can therefore be read without anothercwdcheck. This contradicts the claim that rotation or truncation prevents foreign transcript adoption. Reset verification before reading replacement content, or bind verification to the file identity or generation. The current shared handle preservesstate.verifiedacross the reset. (raw.githubusercontent.com)
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/skills/skills/conciv-harness/references/transcript-history.md`:
- Around line 90-104: Document the interrupted-run recovery behavior for
transcriptHistory when harnessSessionId is absent: preserve the run-message row
and plain-fold its text into session history when native transcript history is
unavailable or rich-history folding cannot run; delete the row only when native
transcript history is addressable and rich-history folding succeeds.
- Around line 69-77: Update the TranscriptHandle documentation around read() to
state that it returns the complete cumulative folded message list, not only
newly read messages. Document that consumers must replace their current messages
with the returned list, and that truncation or rotation resets the fold and sets
replaced: true; reference the existing source.parser.messages(state.fold) and
replaced behavior without changing implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 523e5099-2c82-4c3a-9c51-c7292d869d9e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/conciv-skills-pack.md.fallowrc.json.github/workflows/ci.ymlpackage.jsonpackages/publish/src/guards.tspackages/skills/package.jsonpackages/skills/skills/_artifacts/domain_map.yamlpackages/skills/skills/_artifacts/skill_spec.mdpackages/skills/skills/_artifacts/skill_tree.yamlpackages/skills/skills/conciv-debug/SKILL.mdpackages/skills/skills/conciv-develop/SKILL.mdpackages/skills/skills/conciv-develop/references/rpc-and-testing.mdpackages/skills/skills/conciv-develop/references/tool-contract.mdpackages/skills/skills/conciv-harness/SKILL.mdpackages/skills/skills/conciv-harness/references/transcript-history.mdpackages/skills/skills/conciv-setup/SKILL.mdpackages/skills/src/check-references.tspackages/skills/src/cli.tspackages/skills/test/check-references.test.tspackages/skills/tsconfig.jsonpackages/skills/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- package.json
- .fallowrc.json
- .changeset/conciv-skills-pack.md
- packages/skills/package.json
- packages/skills/vitest.config.ts
- packages/skills/tsconfig.json
- packages/skills/skills/conciv-develop/references/rpc-and-testing.md
- packages/skills/skills/conciv-develop/references/tool-contract.md
- packages/skills/skills/_artifacts/skill_tree.yaml
- packages/skills/test/check-references.test.ts
- packages/skills/skills/conciv-debug/SKILL.md
- packages/skills/skills/conciv-develop/SKILL.md
- packages/skills/skills/_artifacts/skill_spec.md
- packages/skills/src/cli.ts
- packages/skills/skills/conciv-setup/SKILL.md
- packages/publish/src/guards.ts
- packages/skills/skills/conciv-harness/SKILL.md
- .github/workflows/ci.yml
- packages/skills/skills/_artifacts/domain_map.yaml
- packages/skills/src/check-references.ts
| and it returns a `TranscriptHandle` (`revision()/read()/close()`) that: | ||
|
|
||
| - tracks a byte `offset` and re-reads only the new tail on each `read()` call, not the whole file | ||
| (`readBytes(found.path, state.offset, ...)`, `packages/harness/src/_shared/jsonl-handle.ts:125`); | ||
| - only counts **complete** lines (splits on the last `\n` in the buffer, | ||
| `completeLines`, `packages/harness/src/_shared/jsonl-handle.ts:65-70`) so a partial write mid-flush | ||
| never corrupts the fold; | ||
| - resets to offset 0 and re-empties state if the file shrank (`found.info.size < state.offset`, | ||
| `packages/harness/src/_shared/jsonl-handle.ts:120-123`) — handles log rotation/truncation; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document that read() returns a cumulative snapshot.
The handle reads only new bytes, but read() returns the complete folded message list. When truncation resets the fold, the result also carries replaced: true. Consumers must replace their current messages instead of appending the returned list. Without this contract, log rotation can duplicate transcript messages. The shared handle currently returns source.parser.messages(state.fold) together with replaced. (raw.githubusercontent.com)
Proposed documentation update
- tracks a byte `offset` and re-reads only the new tail on each `read()` call, not the whole file
+ tracks a byte `offset` and reads only the new tail, but each `read()` returns the complete folded message snapshot
+ When `read()` returns `replaced: true`, replace the consumer's current message list instead of appending it📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| and it returns a `TranscriptHandle` (`revision()/read()/close()`) that: | |
| - tracks a byte `offset` and re-reads only the new tail on each `read()` call, not the whole file | |
| (`readBytes(found.path, state.offset, ...)`, `packages/harness/src/_shared/jsonl-handle.ts:125`); | |
| - only counts **complete** lines (splits on the last `\n` in the buffer, | |
| `completeLines`, `packages/harness/src/_shared/jsonl-handle.ts:65-70`) so a partial write mid-flush | |
| never corrupts the fold; | |
| - resets to offset 0 and re-empties state if the file shrank (`found.info.size < state.offset`, | |
| `packages/harness/src/_shared/jsonl-handle.ts:120-123`) — handles log rotation/truncation; | |
| and it returns a `TranscriptHandle` (`revision()/read()/close()`) that: | |
| - tracks a byte `offset` and reads only the new tail, but each `read()` returns the complete folded message snapshot | |
| (`readBytes(found.path, state.offset, ...)`, `packages/harness/src/_shared/jsonl-handle.ts:125`); | |
| - When `read()` returns `replaced: true`, replace the consumer's current message list instead of appending it | |
| - only counts **complete** lines (splits on the last `\n` in the buffer, | |
| `completeLines`, `packages/harness/src/_shared/jsonl-handle.ts:65-70`) so a partial write mid-flush | |
| never corrupts the fold; | |
| - resets to offset 0 and re-empties state if the file shrank (`found.info.size < state.offset`, | |
| `packages/harness/src/_shared/jsonl-handle.ts:120-123`) — handles log rotation/truncation; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-harness/references/transcript-history.md`
around lines 69 - 77, Update the TranscriptHandle documentation around read() to
state that it returns the complete cumulative folded message list, not only
newly read messages. Document that consumers must replace their current messages
with the returned list, and that truncation or rotation resets the fold and sets
replaced: true; reference the existing source.parser.messages(state.fold) and
replaced behavior without changing implementation.
| ## `TranscriptFailure`: three reasons, not a thrown error | ||
|
|
||
| ```ts | ||
| export const TRANSCRIPT_FAILURES = ['missing', 'unreadable', 'corrupt'] as const | ||
| export type TranscriptFailure = {ok: false; reason: TranscriptFailureReason; detail: string} | ||
| ``` | ||
|
|
||
| (`packages/protocol/src/harness-types.ts:155-161`.) Only `TranscriptHandle`'s own methods | ||
| (`revision()`/`read()`) return this discriminated union by signature — `HarnessHistory`'s methods | ||
| (`messages()`, `list()`, and the optional refinements) don't. The `resolvePath()`/`verifyHead()` hooks | ||
| you hand to `makeJsonlHandle` also return it (`JsonlSource`, `packages/harness/src/_shared/jsonl-handle.ts:21-25`) | ||
| — build one with `transcriptFailure(reason, detail)` (`packages/harness/src/_shared/jsonl-handle.ts:27-29`). | ||
| Use `'missing'` when the file/session doesn't exist yet or belongs to a different project, `'unreadable'` | ||
| for I/O errors, `'corrupt'` when the file exists but its content fails a structural check (e.g. no | ||
| `session_meta` envelope in the head). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document interrupted-run recovery when harnessSessionId is absent.
When transcriptHistory: true has no recorded harnessSessionId, the run-message row is the only durable copy of an interrupted text turn. Plain-fold that row into session history and keep it. Delete the row only when native transcript history is addressable and rich-history folding is available. Otherwise recovery can delete visible text.
Based on learnings: when transcriptHistory:true has no harnessSessionId, preserve the run-message row and plain-fold it; delete it only when native transcript history is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/skills/skills/conciv-harness/references/transcript-history.md`
around lines 90 - 104, Document the interrupted-run recovery behavior for
transcriptHistory when harnessSessionId is absent: preserve the run-message row
and plain-fold its text into session history when native transcript history is
unavailable or rich-history folding cannot run; delete the row only when native
transcript history is addressable and rich-history folding succeeds.
Source: Learnings
What
New published package
@conciv/skills: four consumer-facing tanstack-intent skills teaching external coding agents how to build with conciv, plus CI linting that catches drift when the API changes.conciv-setup— install/mount per bundler (@conciv/itvs manual@conciv/embed), harness selection, config surfaceconciv-develop— extension authoring (defineExtension/defineTool/defineAttachment, surfaces, RPC, testkit)conciv-harness— capability-typedHarnessAdapter/defineHarnesscontract, sandbox-cwd landmineconciv-debug— widget-missing / tool-hang / approval-stall / SSE symptom→cause→fixDrift linting
intent validate --checkblocking inrepo-checks(gates PRs and, via CI reuse, releases)check:refs(newpackages/skills/src/check-references.ts): verifies every skill's## Sourcespath andfile.ts:Ncitation still resolves against source; covers ALL repo skills (this pack +packages/client/skills+ harness-plugin skills); proven to fail on a mutated citationintent stale --jsonnon-blocking (fixed-version lockstep makes version-drift signal meaningless pre-1.0); flip to blocking at 1.0Notes
apps/sitedocs — the develop author founddocs/extending/*.mdxdescribes an older extension API (follow-up: refresh those docs)@conciv/widget/globalpackage for webpack manual injection (follow-up)skills/only (no_artifacts, nosrc); consumer fixture verifiedintent list/loadend-to-endrelease:check131/131 green; fallow audit pass, nothing introduced🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
@conciv/skillspackage with guidance for setup, extension development, harness adapters, and runtime debugging.Documentation
Quality Improvements