Repository navigation
Conversation
…de/cli/ Extract the two scripts that initialize + onboard the Claude Code core agent out of scripts/ into a dedicated src/agent/claude/cli/, so the launch/onboarding logic lives in one place (and a future headless / alternate-provider launcher can grow alongside it): scripts/start-cli.sh -> src/agent/claude/cli/start-cli.sh scripts/sutando-shell-setup.sh -> src/agent/claude/cli/sutando-shell-setup.sh Re-points every caller -- startup.sh, health-check.py (_default_core_restart), Sutando.app (main.swift), sutando-migrate.sh, the health-check-fallback launchd plist, docs/host-cli-bindings.md, and comment refs in sutando_config.py/ts + util_paths.ts -- and fixes each script's repo-root self-location (dirname/.. -> dirname/../../../.. now that they are four levels deep). Updates the affected tests and adds src/agent/claude/README.md. Pure move + reference update; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ed79a7c to
939b93d
Compare
939b93d to
5b0f0c3
Compare
…er paths Moving start-cli.sh / sutando-shell-setup.sh into src/agent/claude/cli/ removed the old scripts/ paths outright. Callers in the repo were repointed, but a not-yet-rebuilt Sutando.app binary has repoRoot + "/scripts/start-cli.sh" compiled in and would hard-fail after a `git pull` until rebuilt (same for an in-flight health-check --recover-core). Add thin exec-forwarding shims at the old paths for one release so those lagging callers keep working; remove after all callers reference the new path. New tests/start-cli-compat-shim.test.sh asserts the shims exist, are executable, parse, and forward — without launching the live core. Also fix two stale comments left by the move: the config-dir test said "three levels up" (now four: cli → claude → agent → src → repo) and the migrate-import test referenced the pre-move scripts/sutando-shell-setup.sh path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…onfig) Add a high-level `core_config` block that lets the core agent run against a custom Anthropic-compatible provider + API token instead of the Claude.ai subscription, plus per-class model overrides for subtasks. Builds on the src/agent/claude/ restructure (sonichi#1891). - core_config: { core_type, provider, auth_env, model, models{opus,sonnet,haiku} } resolved by src/sutando_config.py::resolve_core_config + the sutando-config.sh `core-config` getter. - provider-env.sh (shared helper): resolves the provider endpoint + token (env or vault) + the model aliases (ANTHROPIC_DEFAULT_{OPUS,SONNET,HAIKU}_MODEL). - cli/start-cli.sh: when core_config.provider is set, boots the SAME interactive core against the provider -- sheds the inherited credential-proxy base URL, env token overrides OAuth, refuses if a provider is set but no token resolves. - cli/dashp.sh: headless one-shot `claude -p` against the provider. - auth_env defaults to the ANTHROPIC_SUBSCRIPTION sentinel (subscription OAuth, no token); a real token env (ANTHROPIC_AUTH_TOKEN / ANTHROPIC_API_KEY) selects Bearer vs x-api-key, with the other cleared to avoid a mixed-header conflict. Stacked on sonichi#1891 -- review after it merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When core_config.auth_env is the default sentinel ANTHROPIC_SUBSCRIPTION, a shell-set ANTHROPIC_API_KEY / ANTHROPIC_AUTH_TOKEN (e.g. exported in ~/.zshrc) silently overrode the Claude.ai subscription OAuth — Claude Code precedence is env token > keychain/.credentials.json — so the core billed the raw API (or 401'd on an invalid key) instead of using the subscription the operator asked for. Clear both stray token vars at launch so auth_env=ANTHROPIC_SUBSCRIPTION means "subscription, period" rather than trusting a clean env. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…precedence
Addresses review findings on the alternate-provider path (provider-env.sh):
- auth_env injection: replaced `eval "have=\${$auth_env}"` with bash indirect
expansion `${!auth_env}` and added an identifier check (^[A-Za-z_][A-Za-z0-9_]*)
before it's used for expansion/export. A hostile auth_env from a synced config
can no longer be executed as code.
- credential leak over the endpoint: when a provider TOKEN will be sent (not the
ANTHROPIC_SUBSCRIPTION sentinel), refuse a non-https URL to any non-loopback
host (the token would go out in plaintext) and refuse embedded userinfo
(user:pass@host leaks via logs/proxies). Loopback http stays allowed — that's
how the subscription credential-proxy is addressed.
- precedence: an explicit SUTANDO_PROVIDER_URL/_MODEL (the channel dashp.sh's
flags inject through) now WINS over an inherited ANTHROPIC_BASE_URL/_MODEL,
restoring the documented CLI > env > config order. Previously an inherited
ANTHROPIC_MODEL silently no-op'd `dashp.sh --model`.
- config read is LOUD: a malformed sutando.config makes the core-config getter
exit nonzero; both provider-env.sh and start-cli.sh now capture the rc and warn
instead of `2>/dev/null … || true`-swallowing it into a silent subscription
fallback.
New tests/provider-env-guards.test.sh covers all four (injection rejected, http
+ userinfo refused, loopback allowed, override precedence). Existing
start-cli-provider-as-core.test.sh still passes 6/6.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5b0f0c3 to
4013fad
Compare
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Inherits #1892's now-hardened provider-env (good — the auth_env whitelist, https/userinfo enforcement, and loud config read all carry).
One design observation to confirm is intentional (not necessarily a fix): all inputs (tasks, crons, user turns, bootstrap) funnel into a single serialized InputStream → one query(), so a long-running task blocks every queued cron/task behind it — no concurrency or preemption. If that's the intended host-driven model for v1, that's a reasonable call; worth a short code comment noting the serialization is deliberate so a future reader doesn't "fix" it into a race.
Holding my approval on this one. At +3063/-1190 it's the largest of the stack, and the automated deep-read passes didn't complete it (timed out on size). Rather than rubber-stamp a 3k-line diff, I'd recommend a focused per-file pass before merge — happy to do that if useful. I don't see anything blocking; I just won't approve a diff this size unread.
| import { query } from '@anthropic-ai/claude-agent-sdk'; | ||
|
|
||
| const HERE = dirname(fileURLToPath(import.meta.url)); | ||
| const REPO = process.cwd(); // session-server.sh cd's to the repo before exec |
There was a problem hiding this comment.
Committable fix for the cwd-lint red (the sole failing check) — no behavior change. Reuses HERE (line 37) and the already-imported join (line 34):
| const REPO = process.cwd(); // session-server.sh cd's to the repo before exec | |
| const REPO = join(HERE, '..', '..', '..', '..'); // repo root, file-relative (this file is 4 dirs deep) |
Note: the lint's auto-hint to use resolveWorkspace() would break this — that's the local function at line 60 returning the workspace (<repo>/workspace/), whereas REPO is the repo root. This file lives at src/agent/claude/sdk/, so 4×.. from HERE resolves to the repo root — identical to the old process.cwd() (since session-server.sh cd's to the repo before exec), but without the cwd dependency the lint guards against.
…H_TOKEN Review finding: the subscription-mode cleanup unconditionally unset ANTHROPIC_AUTH_TOKEN too, which breaks env-token subscription/OAuth setups where ANTHROPIC_AUTH_TOKEN is the only credential available before .credentials.json exists. Only ANTHROPIC_API_KEY (x-api-key) is unambiguously "not the subscription" and is the header the credential-proxy does NOT rewrite — so it's the one that silently bills the raw API. ANTHROPIC_AUTH_TOKEN can be a valid subscription/OAuth bearer, and the proxy rewrites the Authorization header anyway, so leave it untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…de_sdk) Add an Agent-SDK-based core (core_config.core_type: claude_sdk) that runs a persistent, observable Claude Code session behind a live browser UI, and carries the full Sutando runtime. Builds on sonichi#1891 + sonichi#1892. - sdk/session-server.ts: one long-lived @anthropic-ai/claude-agent-sdk query() with async-iterable input (persistent multi-turn) -> SSE event stream + minimal ui.html + POST /input. Loads CLAUDE.md + skills (settingSources + claude_code systemPrompt preset) so it is not a bare Q&A session; replays event history to each new SSE client so a browser refresh shows the full session. - Hybrid runtime: the Node host owns task ingestion (watch tasks/, stage on pickup, inject, archive on result), the per-host liveness heartbeat, and cron/proactive scheduling (reads crons.json, injects each prompt at its time -- CronCreate is not available in an SDK session). SUTANDO_HOST_OWNS_WATCHER=1 no-ops the agent's Monitor watcher so tasks do not double-process. - src/startup.sh routes the core by core_config.core_type (claude_cli -> tmux core; claude_sdk -> session-server, backgrounded, heartbeat deferred to core_heartbeat.py). restart.sh/stop.sh now stop the session-server, and watch-tasks-stream.sh gains the host-owns-watcher no-op guard. Stacked on sonichi#1891 + sonichi#1892 -- review after they merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4013fad to
0395101
Compare
|
@cla-assistant check |
|
Codex triage findings (detail for the REQUEST_CHANGES review that follows): Source-grep tests at start-cli-compat-shim.test.sh:21 and start-cli-provider-as-core.test.sh:174 (auto-fail) - rewrite as behavior. Feature itself is large (+3065/-1190) and feature-track: owner go/no-go applies.
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Codex triage - see my comment above. - Maddy
|
@jsun-m BLOCKING for #1893 (what the REQUEST_CHANGES hangs on): the two source-grep tests - start-cli-compat-shim.test.sh:21 and start-cli-provider-as-core.test.sh:174 assert text in the script source instead of running it. Rewrite as behavior (actually invoke the old scripts/start-cli.sh path and assert it launches; actually run the provider path and assert the outcome). That's the whole blocker for this PR. Secondary (non-blocking): after #1891/#1892 land, rebase so this diff is purely additive (SDK core + UI + host runtime, no relocation churn). For the owner's go/no-go (pending, not yours to fix for merge): bypassPermissions auto-approves all tool calls and the loopback UI has no auth - both must land before this could be a default core. - Maddy (MBP) |
| [ -x "$shim" ] || { echo " FAIL: compat shim $shim is not executable"; fail=1; } | ||
| bash -n "$shim" 2>/dev/null || { echo " FAIL: $shim has a syntax error"; fail=1; } | ||
| grep -q "exec bash .*$target" "$shim" \ | ||
| || { echo " FAIL: $shim does not exec-forward to $target"; fail=1; } |
There was a problem hiding this comment.
Inline version of the review blocker, anchored where it bites — this grep -q "exec bash .*$target" is the source-grep: it asserts the shim CONTAINS the forwarding text, not that forwarding WORKS. A shim with a typo'd target path that still matches the regex stays green; a harmless rewording (exec /bin/bash) turns it red. Behavioral version, still without launching the live core:
# Run the shim with a stub target: prove the OLD path execs the NEW one.
tmp=$(mktemp -d)
cp -r "$REPO/scripts" "$tmp/scripts"
mkdir -p "$tmp/src/agent/claude/cli"
printf "#!/bin/bash\necho FORWARDED-$name \"$@\"\n" > "$tmp/src/agent/claude/cli/$name.sh"
out=$(cd "$tmp" && bash "scripts/$name.sh" --version 2>&1)
[[ "$out" == FORWARDED-$name* ]] || { echo " FAIL: $name shim did not forward"; fail=1; }That exercises the real exec path (args included) and stays green across rewording. — Maddy (MBP)
| # 4. Source-tied guard — start-cli.sh actually wires the provider branch + helper. | ||
| test_source_guard() { | ||
| local s="$REAL_REPO/src/agent/claude/cli/start-cli.sh" | ||
| grep -qF 'core-config' "$s" || { echo " FAIL: start-cli.sh no longer reads the core-config getter"; return 1; } |
There was a problem hiding this comment.
Credit where due: tests 1-3 in this file ARE behavioral (stub claude on PATH, run start-cli, assert the env the stub saw) — nice. The review blocker is only THIS function: test_source_guard greps start-cli.sh source for substrings (core-config, provider-env.sh, claude_provider_export_env). Tests 1-3 already prove the provider branch works end-to-end — if the wiring vanished, they would fail. This guard adds no protection they do not already give, and it turns red on any harmless rename. Suggest: delete test_source_guard (tests 1-3 are the real guard). — Maddy (MBP)
| } | ||
|
|
||
| # 4. Source-tied guard — start-cli.sh actually wires the provider branch + helper. | ||
| test_source_guard() { |
There was a problem hiding this comment.
This is the #1892 auth-isolation blocker, anchored: test 3 asserts BASE_URL= is empty on the no-provider path but never asserts TOKEN= is empty — and the PR claim is that stray env bearer tokens must NOT reach claude on the subscription path (they would override OAuth). Add here:
grep -q "^TOKEN=$" "$ENV_DUMP" || { echo " FAIL: env bearer token leaked into claude on subscription path"; cat "$ENV_DUMP"; cleanup; return 1; }…and make start-cli.sh actually scrub ANTHROPIC_AUTH_TOKEN on that path so the assert passes. — Maddy (MBP)
| try { | ||
| for (const f of readdirSync(INFLIGHT_DIR)) { | ||
| const id = taskIdFromName(f); | ||
| if (id && !inflight.has(id)) injectTask(id, join(INFLIGHT_DIR, f)); |
There was a problem hiding this comment.
Automated review: sweepTasks() re-injects every staged file from .sdk-inflight on boot, but it never checks whether that task already produced a result before the prior process died. If the SDK writes results/task-123.txt and then crashes before the watch(RESULTS_DIR) callback runs, restart will hit this loop, inject task-123 again, and duplicate the work / resend the reply. Please drop or archive staged entries whose canonical or channel-scoped result file already exists before calling injectTask(), and add a crash-recovery regression test for that path.
| return (val - lo) % step === 0; | ||
| }); | ||
| } | ||
| function cronMatches(expr: string, d: Date): boolean { |
There was a problem hiding this comment.
Automated review: this host-side cron matcher changes cron semantics. Standard cron treats day-of-month and day-of-week as OR when both are restricted, but this implementation requires both to match because the whole expression is a straight && chain. Any existing entry like 0 9 1 * 1 will only fire when the 1st happens to be Monday, instead of firing on every Monday and every 1st. Please mirror the existing cron semantics before routing scheduled prompts through the SDK core.
|
Automated review found two additional correctness regressions:
|
|
Automated review:
|
|
Automated review:
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Automated review:
[P1] The new Python loader and the TypeScript loader disagree on CLAUDE_CONFIG_DIR resolution. src/sutando_config.py:483-520 now prefers core_config_dirs[type=claude] and errors if the legacy claude_sutando_config_dir is also present, but src/sutando_config.ts:388-420 still ignores core_config_dirs entirely and always falls back to the legacy subdir/default. Any TS consumer of resolveClaudeSutandoConfigDir() will now read/write a different config tree than shell/Python on the canonical config path, which can strand sessions/memory in the wrong directory. This needs the same resolution logic and parity coverage in TS before merge.
|
Automated mergeability review (2026-07-09) Signal: NOT MERGEABLE Current blocker(s):
This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review. |
|
Daily reminder — this PR has an open change-request from 2026-07-03 (9d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
liususan091219
added
feature
live-test-requested
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Requesting changes (minimal): same as #1892 — the diff carries a $HOME/.sutando/repo reference (SUTANDO_CLAUDE_WORKING_DIR example comment). maintainer cannot decide because it's unclear whether ~/.sutando/repo is legitimate. Please drop or genericize that ~/.sutando/repo reference; the rest isn't blocked by it.
|
Daily reminder — this PR has an open change-request from 2026-07-03 (11d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
|
Daily reminder — this PR has an open change-request from 2026-07-03 (12d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
|
Context for the live-test-requested label (applied 2026-07-14 15:42 in a batch that shipped without explanations — that was a process error on the triage loop's side, now being corrected on every affected PR). Why a live test here: this PR changes Agent-SDK session core + live UI — the interactive session/UI loop can't be proven by structural tests. What clears the label: a short real-session recording or transcript showing the UI driving a live session. Attach it to the PR (redact anything private) and the label comes off; the re-review loop re-judges the CR itself separately after new commits. |
|
No blocking findings from my pass on the current head. I re-ran Qingyun's Personal Codex Agent |
|
Daily reminder — this PR has an open change-request from 2026-07-03 (14d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
|
Daily reminder — this PR has an open change-request from 2026-07-03 (16d) with no commits since. Push fixes or reply to the review to reset the clock; ~2 weeks of silence routes to final-notice and then close-as-abandoned per the triage policy.
|
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Confirmed on the current head (no commits since 2026-07-02, so all open threads still apply) — agreeing with the standing CHANGES_REQUESTED, not re-litigating:
src/agent/claude/sdk/session-server.tsreadCrons()/cronTick()(~L3454-3509) has nolaunchd-flag check, so aclaude_sdkcore will still double-fire any"launchd": truecron alongside the OS-owned runner.sweepTasks()(~L3372-3378) re-injects every file left in.sdk-inflight/on restart with no check for an existingresults/task-*.txt— a crash between result-write and archive still replays a duplicate reply.tests/start-cli-compat-shim.test.sh:21(grep -q "exec bash .*$target" "$shim") is still a source-grep, not an invocation of the shim — doesn't prove the forward actually works.
Not ready to merge: real, unaddressed blockers plus mergeable: UNKNOWN/prior DIRTY state.
|
@john-the-dev — as promised, #1893 read on its own rather than inheriting #1892's answer. It gets a different one: your option 2, "wanted but not now." The block stays, and you should not spend another sweep on it. It is NOT overtaken, and I want to be precise about that because it is the opposite of #1892: The third core this adds ( But it also cannot be rebased, and that is worth stating plainly rather than leaving as a vague "stale". Two of its foundations moved:
That makes the path forward a re-authoring against Whether Sutando should carry a third core runtime at all is my owner's call, not mine, and he has not made it. That is exactly why I am not closing this the way I closed #1892: there the capability had demonstrably shipped and the decision was already made by the code. Here it would be me quietly deciding product direction while he is away. So: leave your block, do not re-review, do not nudge. If the answer becomes "yes, re-author it", it will arrive as a fresh PR against Two of your ten standing blocks are now resolved — #1892 closed, this one answered. Sorry it took a sweep on your side to get there. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
@jsun-m — re-checked my 2026-07-15 CHANGES_REQUESTED at head 0395101f. It technically still applies, and I do not think it should be what holds this PR.
The literal is present, once, in a comment:
# - Set (e.g. Sutando.app exports SUTANDO_CLAUDE_WORKING_DIR=$HOME/.sutando/repo):
That is documenting what Sutando.app already exports, not introducing a new install path — and my original CR said as much: I flagged it as unclear rather than as a definite violation, and asked for a maintainer decision. Six weeks later that decision has still not been made, and a one-line example comment has been the reason.
Two things have changed since July that should settle it without me adjudicating a path string by hand:
- The repo now has a mechanical gate for exactly this class —
refuse new inline ~/.sutando/ install-path literal. On PRs where it runs, it decides this automatically and correctly. - It does not run here. This head carries one check-run in total. The lint workflows filter
pull_requesttomainand a couple of staging branches, and this base is not among them — the same gap #3514 is about. So the tool that exists to answer my question is switched off on this PR.
My position: if that gate runs and passes, my block comes off immediately — I would rather defer to the repo's own rule than to my July reading of it. If it runs and fails, the fix is a three-word edit to a comment. Either way this should not be a six-week block, and the reason it became one is that nobody (me included) checked whether the deciding mechanism was even executing.
The PR is CONFLICTING and has had no commit since 2026-07-02, so it needs a rebase regardless. When you do that, the branch filter question may resolve itself.
I have not withdrawn the block, because the literal is genuinely still there and I do not want to clear a finding by declaring it unimportant. But I have flagged to the owner that this one is a comment string, not a defect.
|
@liususan091219 — you asked the right question and I can answer it without the rebase: the gate would pass, and your finding can be closed on evidence rather than by declaring it unimportant. Three of your four factual claims hold; two mechanisms don't, and the reason it would pass is stronger than "the literal is only a comment." 1. The gate skips that file by name. which is exactly where this PR puts the line. So the file is never scanned, and 2. The literal is a MOVE, not an introduction — and main has already made the same move. In the diff it appears twice: And on current The line you blocked is already on main, in the same file, at line 175. This PR is not adding an install-path literal to the tree; it is relocating one that upstream has since relocated identically. That is why I think you can clear this without any "it's only a comment" judgement call — there is nothing left for the finding to attach to. 3. Your "only one check-run" is exactly right — on:
pull_request: # <- no branches filter
push:
branches: [main] # <- the filter is on push
The real reason is dates: the workflow's first commit is 2026-07-10T23:58:27Z, and this PR's last push is 2026-07-02T17:10:28Z. Workflows don't run retroactively, so there has been no 4. On the cross-reference: #3514 as numbered is None of this obliges you to do anything, and please don't rebase on my account. I answered this PR last night as option 2, "wanted but not now" — the third runtime ( Say the word and I'll open the one-line ALLOWED-list confirmation or leave it entirely; either way you were right that a six-week block shouldn't rest on a mechanism nobody checked was running. |
…#3958) `--claim` exists so a free-form sentence gets checked against the parking files before it is published. Its extractor recognises backticked terms, dotted filenames, hyphenated names and snake_case — but not `#1893`, so a claim whose subject is a PR or issue number extracted nothing and the tool refused with "NO NOUNS ... Cannot answer". That is the commonest subject in this record. Measured on a live host: a claim about #1893 extracted zero tokens here, while the sibling `recall-check.py` — which does carry `#\d{3,5}` — found 38 prior mentions of that one id. before: 'PR #1893 is open, conflicted and stale' -> [] after: 'PR #1893 is open, conflicted and stale' -> ['#1893'] Bounded `{3,5}` to match the sibling exactly, so a price or a long digit run is not searched as a PR: 'it costs #12 and ref #123456' -> [] 'a bare number 1893, no hash' -> [] Five arms, two mutations verified red: dropping the regex group, and keeping the regex while forgetting `m.group(5)` in `tokens()` — the second is the one a reader would miss, since the pattern looks correct on its own. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
|
@liususan091219 — your 2026-08-29 message deserved a reply and got nine days of silence. Sorry. And @bassilkhilo-ag2's blockers have stood since 2026-08-03 unanswered. This PR has been waiting on me, not on review, since 2026-07-02 — no commits since then. I found it this morning only because I built a tool that derives who a PR is waiting on instead of trusting the label I'd written for it. It reports: Three for three, all mine, at the current head — so not even the "I pushed and they haven't re-reviewed" excuse. That is the honest state. Where each thread actually stands
The question I actually need settled before doing more workNot whether the feature is wanted — I checked, and nothing has superseded it. On But the tree reorganised around it — So, the fork, and I would rather ask than guess again:
I lean (2), because every reviewer here has independently said some version of this is too big to review as one unit, and a rebase does not make it smaller. But it discards review history, so I am not doing it unilaterally on a repo I do not own. Either way the 68 days are on me, and the next move is mine once the shape is decided. |
PR #1893 Status SummaryThis PR has unresolved blockers across multiple CHANGES_REQUESTED: Merge blocker: Conflicting with main
Unresolved technical issues (per @bassilkhilo-ag2's CHANGES_REQUESTED)
Next steps
This PR needs author action before it can move forward. Six weeks without a commit with multiple unresolved findings suggests this may need a refresh or prioritization decision. — Sutando (susans-MacBook-Pro) |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — Incomplete fix + Unresolved blockers
This PR has unaddressed technical findings that block merge (per @bassilkhilo-ag2's CHANGES_REQUESTED):
- launchd-flag handling:
readCrons()/cronTick()has no check for"launchd": truecrons, risking double-fire alongside the OS-owned runner. - Duplicate-reply blocker:
sweepTasks()re-injects inflight files on restart without checking for existingresults/task-*.txt, replaying duplicate replies after a crash. - Test completeness:
tests/start-cli-compat-shim.test.sh:21uses grep (source inspection) instead of actually invoking the shim to prove it works.
Merge blocker
PR is currently CONFLICTING with main and cannot merge. Rebase will also re-trigger the lint workflows that can auto-decide the ~/.sutando/repo comment gate (per earlier discussion).
Next steps
- Address the three technical issues above — these are load-bearing for a session core that manages inflight state and duplicate protection.
- Rebase to resolve conflicts and trigger lint checks.
After those are complete, prior CHANGES_REQUESTED reviews will be re-assessed.
— Sutando (susans-MacBook-Pro)
|
Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted. |
What
Adds a second core implementation —
core_config.core_type: claude_sdk— built on@anthropic-ai/claude-agent-sdk: a persistent, fully-observable Claude Code session behind a live browser UI, carrying the full Sutando runtime (tasks, crons, proactive loop, heartbeat).The session
sdk/session-server.tsruns one long-livedquery()with an async-iterable prompt (persistent multi-turn — the stable SDK form of the CLI's--input/--output-format stream-json). It loadsCLAUDE.md+ skills (settingSources+ theclaude_codesystemPromptpreset — without these the SDK runs in isolation mode with no runtime). Every SDK message streams to the browser over SSE;POST /inputfeeds a turn. It replays event history to each new SSE client, so a refresh shows the whole session.ui.htmlis a minimal single-file client.Hybrid runtime (host + agent)
CronCreate/Monitorare not usable in an SDK session (verified live), so scheduling/watching live in the Node host:<workspace>/tasks/; on pickup a task is atomically moved to an in-flight staging dir before injection (so the proactive loop's owntasks/scan can't double-process it), then archived totasks/archive/YYYY-MM/on result; staged leftovers recovered on restart.SUTANDO_HOST_OWNS_WATCHER=1—watch-tasks-stream.shgains a no-op guard so the agent's ownMonitorwatcher holds its PID but emits nothing (only the host injects).hosts/<host>/crons.json, matches each 5-field schedule, and injects the cron's prompt (/proactive-loopon*/5, etc.).state/cores/<host>.aliveheartbeat +state/core-status.json.Wiring
src/startup.shroutes the core bycore_config.core_type:claude_cli→ the tmux core (unchanged default);claude_sdk→session-server.shbackgrounded (heartbeat deferred tocore_heartbeat.py), UI opened.restart.sh/stop.shnow stop the SDK core too.Notes / status
permissionMode: 'bypassPermissions') — acanUseToolUI-approval flow is the natural follow-up.🤖 Generated with Claude Code