Fix the blind agent-scope prompt, and the OneCLI credential step - #18
Open
OmriBenShoham wants to merge 38 commits into
Open
Fix the blind agent-scope prompt, and the OneCLI credential step#18OmriBenShoham wants to merge 38 commits into
OmriBenShoham wants to merge 38 commits into
Conversation
OmriBenShoham
force-pushed
the
fix/dial-prompt-hoist
branch
from
August 24, 2026 09:44
144b71c to
1408b9c
Compare
OmriBenShoham
changed the title
…dating it `onecli secrets update` accepts a new value only through `--value` (argv); it has no `--file`. So the second run of the skill — the re-onboarding and key-rotation path the prose promises — failed at "Register the credential with OneCLI" with `unknown flag --file`. Delete the stale secret and create a fresh one from the 0600 temp file instead; selective-mode agents pick the new id up in the merge step that follows. The group restart at the end is now `effect:restart`, so it no longer fires after an earlier step bounced (agents keep their image until the gap is fixed and the skill is re-applied). The stub-backed test asserted the bad flag; it now asserts delete-then-create and that `--value` never appears on any OneCLI call. Seen live on an install at 475ba76: first apply succeeded, second failed; with this change both succeed, and a group left out of the list gets `403 blocked_by_policy` from inside its container.
The wizard labels a step by the nearest heading above its fence and adds "(i/n)" when several spinning fences share one, so the sign-in section read "Sign in to Dial (1/12)" … "(12/12)" and the restart section "Restart (1/5)" … — a counter instead of a caption. Give every fence that shows a spinner its own "###" heading (the same change add-dial-tool got), so the wizard prints what each step does. Directive order and bodies are unchanged.
…env vars The opencode payload reads OPENCODE_MODEL_CONTEXT_LIMIT, OPENCODE_MODEL_OUTPUT_LIMIT, and OPENCODE_MODEL_INPUT_MODALITIES (container opencode config writer; host passthrough), but this SKILL.md never documented them: the docs lived only in the providers branch's copy of this file, and the registry catch-up merge (nanocoai#3523) correctly replaced that copy with this one. Restores the three bullets and the attachments caveat (refs nanocoai#3156) verbatim from the branch twin at 82af955 so the vars are documented where installs actually read.
- discover-server.mjs never throws: any .env read failure or internal error resolves to discovery=none so the operator URL prompt is offered - default MM_TEAMSETTINGS_ENABLEOPENSERVER to false; the first browser visit still creates the System Admin, further users are invited - explain the unscoped chat-adapter-mattermost removal guard (typosquat-shaped against the @chat-adapter scope) - REMOVE.md: give the operator the ws-import check, and an explicit guarded teardown for the evaluation server volumes
…026-08-24 versions: repin the agent image to hardened-2026-08-24
…v-vars docs(add-opencode): document the OPENCODE_MODEL_* limit and modality env vars
A step nested with `nc:run effect:step` can print but cannot ask. Its stdout is
a pipe, so isTTY is false and clack never redraws the line: the keys reach the
child, but nothing appears until Enter. /add-dial-tool's agent-scope question is
the first nested prompt in the wizard, so it is the first place this shows.
/add-dial owns the operator's terminal, so it asks the question there and hands
the answer down through a new --input key=value on the driver CLI, which
runSkill already accepted. With no --input the driver calls runSkill(dir, {})
exactly as before, so no other caller changes. The question keeps its place in
the flow, behind "Install the Dial tool now?", and /add-dial-tool keeps asking
it itself when run standalone from a real terminal.
The placeholder is quoted (`--input 'dial_agents={{dial_agents}}'`): it is
substituted as raw text into `bash -c`, so an unquoted answer word-splits. The
validate regexes lose their `\s*` in both skills, so a spaced answer is re-asked
at once instead of bouncing off the child's validate-at-bind three steps later.
With no agent groups yet the question had nothing to ask over, rendering
"Agents on this install: ." — the wizard installs Dial before it creates the
first agent. A has_agents capture now gates the warning, the question and the
nested step, and says plainly that the tool install is being skipped.
Asking from two documents would mean two copies of the consent text, and the
halves that drift are the ones that matter: what the operator is told they are
granting, and which answers are accepted. Both texts move to one file each
(agent-scope-warning.sh, agent-scope-question.txt) that both skills render
through `nc:run capture:`. That needed one engine change: a prompt's question
now renders {{vars}} the way an operator block already did.
OmriBenShoham
force-pushed
the
fix/dial-prompt-hoist
branch
from
August 25, 2026 14:41
f895c8a to
6b4cf40
Compare
…g it
The --input loop skipped anything it did not recognise, which swallows the one
failure this flag makes possible: an unquoted `--input k={{var}}` in a caller's
document word-splits, and the orphaned half arrives as a bare argv entry. The
child then validates a truncated value and defers, blaming an answer the caller
had already accepted.
Every argument after the skill dir must now be a recognised flag; anything else
exits 2 with the usage line, so the split is named where it happens.
OmriBenShoham
force-pushed
the
fix/dial-prompt-hoist
branch
from
August 25, 2026 14:52
ab70b09 to
1226f03
Compare
…-missing-base ci: handle branches without an old image pin
fix(dial): post-merge follow-ups — credential re-run, step captions, registry CI
…main feat(channels): add Mattermost installation skill
…r files
Wizard-driven setup skills carry their steps as nc: directive fences inlined
in SKILL.md, not as helper scripts (the reason add.sh/dial.sh/remove.sh were
removed from add-dial-tool). agent-scope-warning.sh reintroduced that pattern,
and it also passed the rendered group list to a shell as
`sh agent-scope-warning.sh '{{agent_groups}}'`. Group display names are free
text, so an apostrophe (a group named "O'Brien", or "Nano's agent") breaks out
of the single quotes: onboarding fails on an ordinary name, and a crafted name
runs an arbitrary command through the bash -c the engine uses for a step.
Put the consent warning back in an nc:operator block and the question back in
the nc:prompt body, in both add-dial and add-dial-tool. In an operator block
{{agent_groups}} is substituted as display text and never reaches a shell, so
the quoting hazard is gone. The two skills carry the same prose in each
SKILL.md; that duplication is the trade the inlined-directives rule already
chose over a shared external file.
With no {{var}} left in a prompt body, the engine change that substituted
prompt questions (scripts/skill-apply.ts inputMetaOf, and its test) is no
longer needed and is reverted, keeping the shared-engine surface untouched.
The driver's --input guard (refuse an unrecognised argument) and the
no-agent-groups branch are kept. Dead apply-fixtures.json exec matches for the
removed files are dropped.
What: replace the @-import CLAUDE.md composer with composeProjectDoc, which reads every instruction source on the host and writes one flat file. Deletes .claude-fragments/, .claude-shared.md, the symlink sync, and two of the three read-only mounts. Adds a one-shot startup migration that removes the leftover artifacts from upgraded installs. Why: Claude Code drops any @-import that resolves outside the project directory, silently, and a headless container has nobody to answer the approval dialog. Every fragment was a symlink into /app, so the runtime contract, all six module manuals, and the resident credential-gateway prose never reached the model. Only the persona, the one real file, arrived. container/CLAUDE.md is never COPYd into the image, so /app/CLAUDE.md existed only for that dropped import. Composing in Node performs the boundary crossing on the host, where nothing forbids it. The spec (file name, base document, extra blocks, byte cap) is the only thing that differs between providers, so the codex payload calls the same composer. That closes three divergences: cli_scope=disabled now drops the scheduling manual for codex too (it teaches ncl tasks, which is dead without ncl), codex gains the resident skill prose it never composed, and resident skill prose on both providers now honours the group's skill selection instead of inlining every shipping skill. container/CLAUDE.md stays in mountPolicy().surfaceRoots even though nothing mounts it: that entry is what forces install-surface, and therefore read-only, on an operator additionalMount whose allowlisted root covers the project tree. Blast radius: no operator action, no image rebuild. Agents begin attempting capabilities they previously did not know existed; every one routes through an approval gate that has barely been exercised. Verification: pnpm vitest run --exclude scripts/update/transaction.e2e.test.ts (2017 pass), bun test in container/agent-runner (332 pass), tsc --noEmit clean on both trees, eslint src/ at baseline (160 warnings, 0 errors). Composed with the codex payload in a throwaway worktree: 2028 host tests, 366 container tests. Pre-existing red, unrelated and reproduced at ad8837c: the four scripts/update/transaction.e2e.test.ts cases, which need unannotated git tag and fail under a global tag.gpgsign. New cases and their kill conditions: the document carries no @ line; the fragment artifacts are never created; the base document, module manuals and resident skill prose are all present; the composed-at-spawn marker stays the literal first characters (migrate-memory classifies on it); the header stays a comment rather than a heading, which would displace the persona; an unselected skill's prose is omitted; a second compose overwrites rather than appends.
Mechanism: the group folder is bind-mounted read-write at /workspace/agent, so the agent can create entries beside the document the host writes. writeAtomic used `<file>.tmp-<pid>`, and the host is one long-lived process, so that name is both predictable and stable. A symlink pre-planted there is followed by writeFileSync, redirecting the composed document onto any path the host user can write; a directory there makes the finally-rm throw ERR_FS_EISDIR, which rides wakeContainer's transient-retry contract and darks the group on a 60s loop forever. Squatting the whole pid space is ~100k symlinks, so not knowing the pid does not help. Fix: random temp name, `wx` (O_CREAT|O_EXCL) so an existing path including a symlink fails closed, and a cleanup that cannot throw. Mirrors migrate-claude-memory-settings.ts, which runs once at startup and can therefore use pid+time where this needs randomness because it runs per spawn. Also: the skills column was a bare cast one line below the mcp_servers sanitizer. A stored string turned the selection filter into a substring match, so a section the selection never named reached the model with no error anywhere, and a stored null, number, object or malformed JSON threw on every spawn. parseSkillSelection now validates it and falls back to 'all' with a warning, matching the column default. Blast radius: no operator action, no image rebuild, no change to any composed document on a healthy install. Verified: the seven new cases in project-doc-compose.test.ts all go red with either defense reverted and green with both (checked by reverting). 2025 host tests, 333 container tests, tsc clean on both trees, eslint src/ 0 errors. Live: a real agent container plants CLAUDE.md.tmp-1 in its own working dir and the host sees it, confirming the precondition; after the change a restart recomposes a 17,733-byte document with 0 @ lines and 0 stray temp files.
…imit Mechanism: Claude Code "loads a CLAUDE.md file of up to 4 MiB in full and skips a larger file" (code.claude.com/docs/en/memory, read 2026-08-25). Over that cliff the file is not truncated, it is not loaded at all, and nothing is logged anywhere. An agent would receive zero instructions, which is the same silent total loss this composer was rewritten to end. The design note this implements claimed the Claude side needed no cap because Claude Code imposes no limit; that claim is wrong. Fix: DEFAULT_PROJECT_DOC carries maxBytes = 4 MiB, reusing the degradation ladder the codex spec already drives. Two unbounded inputs can reach it, the agent-writable instructions.prepend.md and template-supplied MCP instructions, so the cap is unreachable in normal use and exists to make the pathological case loud rather than silent. Also corrects two claims in comments against the same doc: CLAUDE.md arrives as a user message after the system prompt rather than inside it, and block-level HTML comments are stripped before injection, so the composed-at-spawn header costs no context. Blast radius: none today. The composed document is 17,203 bytes over 269 lines, 0.41% of the cap, so the ladder never runs and output is byte-identical. Worth knowing separately: 269 lines is 1.3x the docs' 200-line adherence advisory, which is the deliberate cost of delivering eight manuals that previously reached the model not at all. Verified: pnpm vitest (2027 pass), tsc --noEmit clean, eslint src/ 0 errors. Two new cases, both red on revert: the default spec carries the cap, and a core that alone exceeds the cap writes an oversized document with an error log rather than throwing, since a per-spawn throw rides wakeContainer's retry and darks the group on a 60s loop forever.
The contract asks for one short new top entry per change; this had grown to six bullets across three commits. Folds them into one, and adds the MCP re-validation side effect, which was a user-visible behavior change for codex groups that no entry mentioned.
scripts/update/transaction.ts:156 derives every /update-nanoclaw migration requirement from added CHANGELOG lines containing [BREAKING]. Without the tag a customized install cuts over silently past a module rename it may import, and past two mounts that tooling may match on. The changelog tag is the wiring, so the skill itself needs no edit. Entry halved to 112 words while keeping the two breaking surfaces and the detect grep, which are the parts an operator acts on.
…vention Three findings from manual review, each fixed as a class rather than an instance. 1. Naming. composeProjectDoc did not follow the repo's <verb>Group<Noun> shape used by initGroupFilesystem, readGroupPersona, stageGroupPersona, resolveGroupTimezone and the two composers it replaces. Renamed composeGroupProjectDoc, on trunk and in the codex payload together. 2. Machinery where prose and an existing pipeline would do. cleanup-legacy-fragments.ts plus its test, its index.ts call and its docs row are deleted, 131 lines. /update-nanoclaw already gates on [BREAKING] changelog lines (scripts/update/transaction.ts:156) and forces an operator acknowledgment, so the migration line now carries the one-line rm instead. The leftovers are inert: nothing reads them. 3. Agent-facing prose. The add-codex preflight goes from five lines to two, and this file's header from twenty to seventeen with the causation corrected — the imports worked until a Claude Code update gated external imports behind an approval a headless container cannot give. Audited the same three classes across the rest of the diff. Two more found and fixed: container_configs.skills was read in two places that had to agree by comment, now one exported parseSkillSelection shared with selectedSkillNames; and the header above. One left deliberately: writeAtomic is a second copy of the one in migrate-claude-memory-settings.ts, and the rule of three is not met, so extracting a shared module now would be premature. Verified: 2024 host tests (three fewer, the deleted migration's), 333 container, tsc clean, eslint src/ back to exactly baseline (160 warnings, 0 errors) since the deleted file carried the only two additions.
Without pipefail, a missing ncl or a dead service exits the probe pipeline with jq's status 0 and binds has_agents="", which matches neither when: guard — every gated directive, including the tool install the operator just approved, lands in skipped and the wizard finishes green. validate:^(yes|no)$ makes bindCapture throw instead, so the failure bounces loudly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sATmMXtSBQkraEGbSTf7w
… --input keys Three hardenings around the new --input handoff: - parseDriverArgv extracted from the CLI block and unit-tested — the argv contract (bare-arg refusal, missing/malformed pairs) had no coverage, so a word-split regression would only surface interactively. - Unknown --input keys are refused (unknownInputKeys): the engine ignores keys it has no prompt for, so a typoed key would leave the child asking that prompt itself — through the pipe, unechoed, the exact failure --input exists to avoid. - Conformance now pins the dial_agents prompt (validate/flags/normalize/ question) and the consent warning to be identical in add-dial and add-dial-tool. The duplication is deliberate (the shared helper file was an injection vector); drift would let the parent accept an answer the child's validate-at-bind rejects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017sATmMXtSBQkraEGbSTf7w
…e column Structure only, no behavior change. Why: the parser reads `container_configs.skills`. That column belongs to container-config.ts, which is also where the second, untolerant reading of it lives — keeping the parser one import away in the composer is what let the two drift apart. What: `parseSkillSelection` moves from `src/project-doc-compose.ts` to `src/container-config.ts`; the composer and `container-runner.ts` import it from there. No cycle: the composer already imports `sanitizeStoredMcpServers` from the same module. Verification: `pnpm exec tsc --noEmit` clean; `pnpm exec vitest run src/project-doc-compose.test.ts src/container-config.test.ts src/container-runner*.test.ts` 106 pass, unchanged.
Why: `configFromDb` cast `JSON.parse(row.skills)`, ten lines before the
composer's tolerant read of the same column. A row that is not valid JSON threw
there first, so the composer's fallback was unreachable at spawn: instead of
losing a filter, the group lost every spawn, and `wakeContainer`'s
transient-retry contract had host-sweep respawning it every 60s forever. Three
of the composer's four corrupt-selection cases are valid JSON and did reach it;
`'{not json'` never could.
What: `configFromDb` calls `parseSkillSelection` (`src/container-config.ts`).
`selectedSkillNames` drops the `JSON.stringify` round trip it used to re-parse
with — `configFromDb` is the only producer of a `ContainerConfig`, so the
value it hands over is already `string[] | 'all'`.
Risk: nothing to deploy. A corrupt row now widens to every skill with one
warning, which is what the composer already did with the same row.
Verification: new case in `src/container-config.test.ts` — red before this
change with `SyntaxError: Expected property name or '}' in JSON at position 1`,
green after. `pnpm test` 2033 pass, 4 fail (`scripts/update/transaction.e2e.test.ts`,
which needs unannotated `git tag` under a global `tag.gpgsign`, red at the base
too). `pnpm exec tsc --noEmit` clean.
fix(compose): inline every instruction source into one project document
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent fixes in the Dial setup path, one commit each.
1. You cannot see what you type at the agent-scope prompt
A step nested with
nc:run effect:stepcan print but cannot ask. Its stdout is a pipe, soisTTYis false and clack never redraws the line — your keys reach the child, but nothing appears until you press Enter./add-dial-tool's which agents may use Dial question is the first nested prompt, so it is the first place this shows./add-dialowns the operator's terminal, so it asks the question there and hands the answer down through a new--input key=valueon the driver CLI, whichrunSkillalready accepted. With no--inputthe driver behaves exactly as before, so no other caller changes.The question keeps its place in the flow — still behind Install the Dial tool now?, so nobody picks agents for a tool they then decline.
/add-dial-toolkeeps its own prompt for a standalone run, where it is asked from a real terminal and echoes normally.The alternative was giving every nested step a real pty, which fixes this for all future skills but adds a compiled native dependency (
node-pty) to the setup path. That did not seem like a trade worth making for a cosmetic problem — happy to revisit if you would rather solve it once at the driver.2. The credential step fails whenever a Dial secret already exists
onecli secrets updatehas no--fileflag — onlycreatereads a file. The step calledupdate --id … --file …, so on any re-onboard, account switch, or key rotation it died on an unknown flag and the tool install stopped.update --valuewould fix the flag but put the API key on argv, which is what the temp file exists to avoid. The existing secret is deleted and recreated from the file instead, so the key still never reaches a command line; the scoping step re-resolves the id from the vault.Checks
typecheck clean ·
skill-conformance+add-dial-tool-scope185/185 green ·setup/prettier output identical to main.