feat(#108): seeding pipeline - CLI provider, domain-rationale, infrastructure domain, concurrency#119
Conversation
LLM_PROVIDER=claude-cli runs filter/distill via 'claude -p' on the operator's subscription (no API key). isRateLimitError/isAuthError/isBatchFatalError detect global LLM failures so the runner can stop the batch instead of flagging each PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…auth --concurrency (PIPELINE_CONCURRENCY, [1,10]) worker pool; --last removes the ~100-PR per-run cap; --resume skips processed PRs; --force bypasses filter. Rate-limit/auth errors stop the batch (exit 2) with a resume hint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… gate; pin Node 22 CI validates the domains/ corpus on PRs and fresh drafts in the pipeline run. .env.example set for claude-cli + Opus 4.8/max + concurrency 4; .nvmrc=22.18.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
551acb2 to
e90aef1
Compare
sugat009
left a comment
There was a problem hiding this comment.
Review: seeding pipeline keystone
Thanks for this, it is a strong, well-structured PR and the parts that matter most are solid. The taxonomy single-sourcing is genuine (the taxonomy-schema-sync test is a real bidirectional drift lock against the live schema.json enums), the VALID_DOMAINS = CHT_DOMAINS fix means infrastructure is accepted everywhere, the Claude CLI provider has a clean injection posture (shell:false, prompt written to stdin, never argv), and it extends the merged #128 pipeline additively with no destructive changes. The docker stack also gets a lot right: no Docker socket mount (with a fail-closed runtime assertion), openssh-client purged, credentials and .git/config mounted read-only, and push blocked at several independent layers.
I went through this across a few dimensions and then re-verified the findings independently, which led me to correct a couple I had initially over-rated (noted at the end for transparency). Posting as comments to open the discussion rather than as a hard block. The one I would genuinely like to settle before merge is the first; the rest are open for debate and I am happy to be wrong on any of them.
Worth resolving before merge
1. Credential exfiltration from the interactive containers (security). In the interactive cht-agent container the ANTHROPIC_API_KEY is in the env and the Claude OAuth token is a readable mount, and the agent can run curl with no egress restriction, so a prompt-injected agent could exfiltrate either in one request. Two things I would rather discuss than assert:
- The automated
run-pipelinepath looks genuinely safe (filter and distill run with--disallowedTools Bash,... --max-turns 1, so Bash/curl are out of the model's context). The exposure I see is the interactivecht-agentcontainer and thedocker exec -it cht-seeder claudelogin session the runbook prescribes. Does that match your model, or is there a control I am missing? - One subtlety so we do not chase a false fix: because the provider always passes
--dangerously-skip-permissions, thesettings.jsonallow list is ignored and only the deny list is enforced. So removingBash(curl:*)from the allow list would not close this. The levers that would are network egress isolation (internal: trueor an egress allowlist) or addingcurlto the deny list, plus keeping secrets out of the agent-readable env. Open to other approaches.
Questions / things that look off (open)
2. Coverage-gate scope. This PR adds the whole .nycrc.json exclude list (there is none on main), and it currently excludes src/llm/providers/** (the new claude-cli.ts, which has a 354-line spec) and src/utils/domain-inference.ts. Net effect: the PR's headline new code and domain-inference.ts do not count toward the 95/85 gate. Was the exclusion meant to be this broad, or a holdover from when providers hit a live API? Since claude-cli.ts is now unit-testable offline, narrowing it would let the new code count.
3. A real usage-limit looks like it can be swallowed. If a Claude usage limit comes back as exit-0 plain-text stdout (no "type":"result" envelope), parseResponse returns it as a success (is_error:false), so the rate-limit classifier never sees it; downstream invokeForJSON then throws the generic "did not contain valid JSON object", which is a non-fatal message, so the batch keeps going against an exhausted subscription. Is that path reachable in practice with claude -p, and if so should a no-result-envelope (or usage-limit text) be treated as batch-fatal? Also probably worth confirming the distiller will not persist the limit text as a draft body.
4. --force --resume --pr <already-done> exits 0. The resume filter drops already-processed PRs before --force is consulted, so re-distilling a specific PR you have already run silently no-ops ("Nothing to process."). Intended, or should --force exempt explicitly named PRs from the resume skip?
5. domain-inference.ts enrichment prompt lists 7 domains. inferUsingLLM (181-230) enumerates 7 and says "pick one of the 7", but VALID_DOMAINS is the 9-value CHT_DOMAINS and the appended example uses infrastructure, so interoperability is never offered and the prompt contradicts its own validator. The distiller path derives the list from CHT_DOMAINS and is fine, so this reads like one spot the single-sourcing did not reach. Deliberate, or worth deriving this prompt from the const too?
6. Seeder volume + token hygiene (smaller). Two operational ones: the seeder's /home/agent/.claude named volume is seeded from the image only on first creation, so later security fixes to settings.json/CLAUDE.md will not reach an existing volume on rebuild; and init-agent-git.sh warns (does not fail) on GH_TOKEN with no actual scope check, so an accidental write-scope PAT would start normally. Both feel worth tightening; curious whether there is a reason they are warn-only.
Checked, looks low-impact (sharing so you know they were considered)
A few I initially flagged higher and walked back after re-checking, for transparency rather than as asks:
- The batch-stop regexes (
/limit reached/,/quota/,/unauthorized/) are unanchored, but the realistic OS errors I worried about do not actually produce those strings (Node EMFILE is "too many open files", ENOSPC is "no space left on device"), and a realgh401/403 is global so stopping is the right call, just with a Claude-flavored message. Low impact; the main residue is that the message can mislead an operator on agh-token failure (it suggestsclaude /login). settings.jsonbeing agent-writable is real but hollow under skip-permissions (only the deny list is live,curlis not denied anyway, and push is blocked at other layers).- The SIGINT/SIGTERM handler registration is fine in practice (the chains are memoized, so ~4 handlers, and a second signal terminates via Node's default action); only a narrow orphan-on-second-signal edge remains.
extractResultEnvelope's multi-object parse is latent only, since the provider only ever passes--output-format json(a single envelope).
Overall this is close to mergeable. Mainly the credential-exfil scope and the coverage-gate scope feel worth settling first, and I am glad to dig into any of the above together.
|
Thanks @sugat009
I'll grab this in #133 , as that may have conflicts and was meant to over-write some of the container portion. Sped ahead to seed the data, and tried to keep them separated.
I'll fix this here. holdover and forgot to fix as was done with Agree on 3,4,5,6 and will grab them here. |
…ructure to VALID_DOMAINS Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ft with a test Derive CHTDomain/CHTService/CHTWorkflow from the CHT_DOMAINS/CHT_SERVICES/CHT_WORKFLOWS const arrays; pipeline.ts re-exports instead of re-declaring; distiller imports CHT_SERVICES. New test asserts schema.json enums == the const arrays. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
schema.json enums, doc-search Record maps, TEMPLATE.md, and distiller/domain-inference tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n't rejected Both VALID_DOMAINS hand-lists now derive from CHT_DOMAINS; updates the validator test message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ons, re-export) Shared linear JSON extractor; CC refactors + justified NOSONAR; more Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…R not honored) Extracted helpers; SonarCloud Automatic Analysis ignores the NOSONAR block, so refactored to <=5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rameters) logPrStart/processBatchItem/runWorker now take a shared BatchCtx, each <=4 params; CC still <=5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rage exclude Proxyquire tests for the run-pipeline helpers + concurrency>1; export 3 helpers for testing. Only providers/open-review-pr/domain-inference stay excluded; coverage gate green Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sume Derive the inference prompt's domain roster/count from CHT_DOMAINS (was hardcoded 7, now 9). Classify plain-text usage-limit/auth CLI output as batch-fatal instead of a success result. Exempt explicitly named --pr from the --resume skip when --force is set (filterResumable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…filterResumable claude-cli.ts 81%->96% (limit/auth, empty-stdout, spawn-error, malformed-envelope paths). istanbul-ignore only timer/signal callbacks; global gate 97/86/97/97 with providers in scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1979c05 to
6a2742a
Compare
|
Removed cht-agent docker files and will merge #133 first. Addressed other feedback 2-6. |
sugat009
left a comment
There was a problem hiding this comment.
Re-reviewed against the actual fix code, the current .nycrc, and the seeder container. 2-6 check out, several with more hardening than I asked for: the usage/auth notice now returns an error envelope via isBatchFatalError (and the patterns do match the real CLI notices); filterResumable exempts a named --pr under --force; the inference roster is derived from CHT_DOMAINS with a DOMAIN_DESCRIPTIONS: Record<CHTDomain, string> that fails to compile on a missing domain (nicer drift-lock than I suggested); and the volume-staleness caveat is documented. Agreed on landing #133 first, and removing the vendored sandbox to layer on FROM cht-agent:local also moves the credential-exfil concern onto #114/#133 where the egress follow-up lives. Nice turnaround.
One sliver of the coverage finding is still open: src/utils/domain-inference.ts. The provider half is fixed (providers/** dropped, claude-cli.ts now gated, 81%→96%), but this file is still in the new exclude list. Since main's .nycrc has no excludes, it is gated on main today, and it has a spec, so the PR effectively turns its gate off in the same change that adds the new roster/inference logic. The domain bug is fixed and compile-locked, so this isn't a correctness risk. Is the exclusion just to dodge the live-LLM call? If so, could we drop the file-level exclude and either mock the call (the provider specs now do this) or scope an istanbul ignore to that one function, so the rest of the file stays gated?
Approving on the strength of everything else; would just like that one exclusion folded in before or shortly after merge. Carryover nit for whenever, not this PR: the rate-limit.ts patterns are still unanchored and the gh-token failure still points at claude /login.
Yeah it was, but i like your suggested approach. |
…cludes Mock @langchain/anthropic to cover the roster/inference logic + parse-error branches (now 100%). Remove domain-inference + open-review-pr excludes; .nycrc matches main. Gate 96.5/85.96/97.3/96.9. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Builds on #109. Enhances the memory distillation pipeline and adds the taxonomy needed to seed agent-memory/.
Pipeline
Taxonomy / output
Tooling
Testing: 528 unit tests; full backlog seeded (338 drafts, validate-schema 0 failures). Reviewed (code/security/architecture) — see follow-up checklist.
Evidence: docs/domain-taxonomy-findings.md.
Review follow-ups (tracked, non-blocking):
3. Merge order
#109 → main (the base pipeline)
#87 coordination — both #87 and this PR edit agent-memory/schema.json (and the 4 contacts services fixes overlap). Cleanest: #87 merges, then rebase this PR onto main so its validator sees our infrastructure/domainFit/workflow enums. If this PR merges first, #87 must rebase and keep our enum additions.
This PR → main.