Community review: deepseek-harness @ master (v0.1.0-rc.5) — findings & suggestions #2450
ardi-zanki
started this conversation in
General
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Hi maintainers! I spent some time reading through the public repo (master @ v0.1.0-rc.5) and, since issues are off here, I wanted to share a few things I noticed. The codebase is honestly in really good shape for a release candidate — I kept this short on purpose.
Hooks can spin forever on a blocking Stop hook. Both bridges hardcode
stop_hook_active: false(packages/hooks/hooks-codex/src/index.ts:257,packages/hooks/hooks-claude-code/src/index.ts:346), so when a Stop hook blocks, the bridge tells the agent to keep going — and the hook never learns it already continued. A hook that always blocks therefore loops until something times out. The related{"continue": false}result is parsed but never actually stops the run either. A counter with a cap on forced continuations would fix the first one.Settings secrets can leak through the redactor. If a secret field sits behind a union/intersection/transform schema,
redactSecretsreturns the value untouched and records nothing (packages/settings/settings/src/redact.ts:86-91). Since the seam promises every wire surface is redacted, rejecting such schemas at registration (fail closed) seems safer than the current fail-open behavior. Related:settings-filedoesn't check file permissions the waycredentials-localdoes (assertOwnerOnly), so a world-readable settings file containing secret fields gets served without complaint.Settings and credentials writes aren't crash-durable.
writeFileAtomic(packages/util/atomic-write/src/index.ts:44) renames the temp file into place without fsync, so a power cut can lose the most recent credentials or settings write. The JSONL session backend fsyncs; these two callers don't. An opt-in fsync there would close the gap.The published landlock-run packages link to a repo that isn't public. All three
native/landlock-run/packages/*packages haverepository.urlpointing atgithub.com/deepseek-harness/deepseek-harness, which 404s for npm users. It should bedeepseek-ai/deepseek-harness— a one-line fix per package.The AGENTS.md layout has drifted a lot. It's missing ~15 groups (client, host, jobs, sandbox, mcp, storage, workspace, …) and misnames two (
self-modification→extensions,support→test-support).packages/README.mdis accurate; linking to it instead of keeping a second list would stop this from drifting again. (vitest.config.ts:175still excludes the oldpackages/self-modification/name too.)The Python SDK has two concurrency rough edges.
start()checksself._procoutside the lock (python/sdk/src/deepseek_harness/client.py:63-85), so two concurrent starts can spawn two runtimes and orphan one. And the reader loop silently drops malformed JSON lines, which hides corrupted runtime frames from the caller.That's everything I'd flag before release. I noticed a few smaller things as well — mostly i18n inconsistencies in the web UI, a stale ACP version string, and some tracked TODOs. Happy to share those too if useful, but I didn't want to bury the real items in noise.
All reactions