feat(kanban): add Modal memo evaluator lane - #147
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e8036618
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4121a8d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
QA: Modal memo-evaluator E2E — fixed and provenI found and fixed four runtime defects on this PR branch in commit
Evidence:
Residual: the current running gateway is on deployed |
The parent's log file handle was only closed on the exception path, leaking an open descriptor on every successful spawn. The child inherits its own dup, so release the parent handle in a finally block to stop fds accumulating as more Modal workers are launched. Addresses the graphite review thread on kanban_modal.py:144.
The worker now loads the research-proxy secret (ANTHROPIC_BASE_URL + ANTHROPIC_API_KEY) and runs claude-fable-5 through the Anthropic-Messages proxy, but the lane docs still named the old bloom-llm-proxy/OPENAI_API_KEY setup. Align the operator instructions with the code so provisioning the documented secret actually works. Addresses the graphite + Codex doc-mismatch threads on kanban-worker-lanes.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 880cf1e702
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM overall — reviewed the Modal memo-evaluator lane against the cache/alternation/narrow-waist/env-config invariants. No prompt-cache risk (the remote worker is a one-shot hermes chat -Q, not a long-lived conversation), no role-alternation change, no new core model tool, and worker_backends correctly lives in config.yaml (not .env). The dispatcher→shim env handoff (HERMES_KANBAN_DB/BOARD/WORKSPACES_ROOT) mirrors the existing _default_spawn pattern, the resolve_worker_backend restriction to memo-evaluator is well-guarded and tested, and the spawn fd-close-in-finally is correct.
Minor (profile-safety): the Modal worker's default profile source is hardcoded to ~/.hermes/profiles/memo-evaluator at
hermes-agent/hermes_cli/kanban_modal_worker.py
Lines 18 to 23 in 880cf1e
HERMES_HOME (e.g. /opt/data) this default won't resolve. It fails loudly with a message pointing at the HERMES_MODAL_MEMO_EVALUATOR_PROFILE override so it degrades gracefully, but the default should derive from the profiles-root helper (_get_default_hermes_home() / "profiles" / "memo-evaluator") instead of Path.home()/".hermes" to be profile-safe by default.
Note: the paid remote path (evaluate_memo running hermes through the Anthropic proxy) is untested by design — the local shim protocol is covered with a fake CLI, which is a reasonable tradeoff given the cost.
1 minor.
There was a problem hiding this comment.
Reviewed the Modal memo-evaluator lane: backend routing (resolve_worker_backend/_configured_worker_spawn), the local shim's DB-lifecycle authority, the remote worker, and the config/docs wiring. The worker_backends toggle lives correctly in config.yaml (not .env), the shim keeps the board DB as the sole lifecycle authority, and role/prompt-cache invariants aren't touched. One profile-safety issue.
Profile-safety: hardcoded ~/.hermes breaks custom/Docker/profile HERMES_HOME deployments. PROFILE_SOURCE defaults to the literal "~/.hermes/profiles/memo-evaluator", which is exactly what rule #9 forbids — on any deployment where the hermes root isn't ~/.hermes (Docker /opt/data, profile-isolated homes), the profile won't be found and the modal.is_local() import-time guard raises RuntimeError, disabling the lane entirely on the dispatcher machine. Resolve the path via hermes_cli.profiles.get_profile_dir("memo-evaluator") (anchored to the hermes root, HERMES_HOME-aware) inside the modal.is_local() guard instead. That also removes the need for the new HERMES_MODAL_MEMO_EVALUATOR_PROFILE env var (non-secret path config, which shouldn't be a HERMES_* env var per rule #4). See
hermes-agent/hermes_cli/kanban_modal_worker.py
Lines 18 to 23 in 880cf1e
1 blocking (profile-safety), 0 minor.
The Modal worker has no Kanban attachments mount, so a memo task whose source doc is uploaded as an attachment would be shipped with only the file path in its brief — the remote container cannot read the bytes and could return a hallucinated complete. Refuse such tasks at dispatch and block them as a capability gap so they are rerouted to a mounted backend. Addresses Codex P1 (kanban_modal_worker.py:139).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a27247be3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Modal worker hardcoded ~/.hermes/profiles/memo-evaluator and read a HERMES_MODAL_MEMO_EVALUATOR_PROFILE env var to override it. That breaks custom/Docker/profile HERMES_HOME layouts (AGENTS.md rule #9) and uses a HERMES_* env var for non-secret path config (rule #4). Resolve the profile via get_profile_dir('memo-evaluator') inside the modal.is_local() guard, which anchors to the profiles root and is HERMES_HOME-aware. Drops the env var entirely. Addresses claude[bot] CHANGES_REQUESTED (profile-safety, blocking).
|
Addressed the claude[bot] profile-safety blocker in |
There was a problem hiding this comment.
Reviewed through the prompt-cache, role-alternation, narrow-waist, env-var, and profile-safety lenses. The memo-evaluator runs as a fresh single-shot hermes chat -Q per task in a Modal container, so there is no long-lived conversation to mutate — criteria 1–3 are clean. worker_backends is correctly a config.yaml key, and no new core model tool is added. One profile-safety issue and one minor nit:
Profile-safety (criterion 9) — hardcoded ~/.hermes. PROFILE_SOURCE defaults to the literal "~/.hermes/profiles/memo-evaluator" at
hermes-agent/hermes_cli/kanban_modal_worker.py
Lines 18 to 23 in 9a27247
HERMES_HOME=<root>/profiles/<name>) or a Docker/custom root (/opt/data), the profiles root is not ~/.hermes, so the default resolves to a path that does not exist and image build fails with the "Set HERMES_MODAL_MEMO_EVALUATOR_PROFILE…" error — exactly the multi-profile fleet case this lane targets. Fix: default from the import-safe helper, e.g. hermes_constants.get_default_hermes_root() / "profiles" / "memo-evaluator" (this is how list_profiles_on_disk/validate_assignee already resolve profiles), keeping the env var only as an optional override. That also removes the criterion-4 concern of shipping a non-secret path as a HERMES_* env var default.
Minor / nit. The MEMO_EVALUATOR_MODEL = "claude-fable-5" constant (
config.yaml model with nothing tying the two together, and test_worker_binds_the_memo_evaluator_model_and_anthropic_proxy_secret just re-asserts that literal — a change-detector that will silently drift from the profile if it is ever re-pinned. Not blocking; worth a comment noting the pin must be updated in lockstep.
1 minor (profile-safety), 1 nit.
There was a problem hiding this comment.
LGTM — reviewed the opt-in Modal memo-evaluator lane against the prompt-cache / role-alternation / narrow-waist / env-var invariants and for correctness.
Invariants: No prompt-cache or role-alternation surface touched (remote runs a fresh one-shot hermes chat -Q -q). No new core tool — this extends the existing dispatcher (_configured_worker_spawn wraps _default_spawn) plus a self-contained CLI module, the correct low rung of the Footprint Ladder. worker_backends is a config.yaml key (not a HERMES_* env var), default local so existing behavior is unchanged. Patch 7 correctly moved profile resolution to get_profile_dir('memo-evaluator') (profile-safety) and dropped the HERMES_* path env var.
Correctness: Verified list_attachments/Attachment.filename, complete_task(metadata=, expected_run_id=), block_task(kind=, expected_run_id=), and capability/transient ∈ VALID_BLOCK_KINDS; connect_closing() honors the injected HERMES_KANBAN_DB/BOARD so the shim writes the right board; modal run -w/--write-result is a real flag and the local_entrypoint returns a str; the expected_run_id guard blocks double-apply on reclaim; and the live-PID claim-extension keeps the shim from being reclaimed mid-run. The attachment guard is a sensible defense against a mount-less hallucinated complete.
Minor (non-blocking): Per the repo's E2E-over-mocks preference, the live Modal path (research-proxy secret + provider wiring) is validated only by provisioning, not exercised — acceptable here since the failure mode is safe (nonzero modal run → task blocked transient) and the CLI contract checks out against Modal's docs.
0 blocking, 1 minor.
… request) Two findings from the re-review on the latest head: 1. Codex P1: an untrusted (prompt-injected/malformed) Modal result could set metadata._staged_artifacts / artifacts to arbitrary host paths, which complete_task turns into attachment records (and remove_attachment would later unlink). Strip reserved host-side directive keys before applying a remote result. 2. graphite (Windows): the brief can be up to 64KB but Windows caps the entire command line at 32,767 chars, so passing the request via --request-json overflows the modal spawn. Pipe the request over stdin instead; worker main() reads sys.stdin. Regression tests cover the metadata stripping and the stdin contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d15b6041c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM — reviewed the Modal memo-evaluator lane: shim/worker split, dispatch routing, worker_backends config, and tests.
Verified against repo invariants:
- Config-not-env: backend switch is a
config.yamlkey (kanban.worker_backends);_deep_merge(config, user_config)lets the user'smemo-evaluator: modaloverride the shippedlocaldefault — no HERMES_* config var, no opt-in reversion. - Profile-safety:
_default_profile_source()resolves viaget_profile_dir('memo-evaluator')under amodal.is_local()guard; no hardcoded~/.hermes. - Untrusted-result hardening:
apply_modal_resultstrips_staged_artifacts/artifactsbeforecomplete_task— confirmed both keys drive host-side attachment create/unlink at kanban_db.py:5680 and :5729, so the strip closes that injection class (regression test proves no attachment from injected paths). - Prompt-cache / role-alternation N/A (worker is a one-shot
hermes chat -Qsubprocess, not a live conversation). Narrow-waist not triggered — no new core model tool. fd-leak finally-close, stdin-over-argv, and attachment-refusal blocks are sound and tested.
Minor (non-blocking): test_worker_binds_the_memo_evaluator_model_and_anthropic_proxy_secret asserts MEMO_EVALUATOR_MODEL == 'claude-fable-5', a tautological freeze of a provider-model literal that mirrors the source constant — will need dual-updating on any model bump, though it's defensible as a guard for the OpenAI→Anthropic migration. Keep or drop as you prefer.
Live Modal invocation is untested (paid remote call, acknowledged), but the shim side is exercised end-to-end against a temp DB.
0 blocking, 1 minor.
A delayed/previous shim that survives a reclaim built expected_run_id from whichever run was current when it finally started, so a stale Modal response could complete a re-claimed NEW attempt. Read the env-pinned HERMES_KANBAN_RUN_ID (set at spawn) as the authoritative run id, mirroring the local worker's _worker_run_id_for guard; fall back to the fresh read only when the env var is absent. Regression test proves a stale run id is rejected and the re-claimed task stays running. Addresses Codex P1 (kanban_modal.py:208).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61f43c1248
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM — reviewed the Modal memo-evaluator lane (local shim + remote worker + dispatch routing) against the prompt-cache, role-alternation, narrow-waist, env-var, and profile-safety invariants.
Highlights that check out:
- No prompt-cache or role-alternation surface: the remote worker is a single-shot
hermes chat -Q, a fresh conversation per Modal call. - Backend selection lives in
config.yaml(kanban.worker_backends); theHERMES_KANBAN_*vars are subprocess handoff that mirrors_default_spawn. The priorHERMES_MODAL_*env var is gone. - Profile resolution uses
get_profile_dir("memo-evaluator")undermodal.is_local()— HERMES_HOME-safe. - The reserved-key stripping is a real defense: I confirmed
complete_taskpromotesmetadata["_staged_artifacts"]/["artifacts"]into host attachments (kanban_db.py:5680), so stripping them from untrusted remote results genuinely prevents arbitrary-file attach/unlink. modal run --write-resultis a real Modal flag andmain()returns a JSON str, so the result protocol is valid. Run-id pinning, stdin-for-large-brief, and attachment refusal are all sound and test-covered.
Minor (non-blocking): in apply_modal_result, the add_comment audit line is posted before complete_task (
hermes-agent/hermes_cli/kanban_modal.py
Lines 157 to 169 in 61f43c1
0 blocking, 1 minor.
The worker runs Hermes with --provider anthropic, but the image installed bare hermes-agent. The native Anthropic SDK is an optional extra, so the adapter raised ImportError and every Modal invocation blocked before evaluating the brief. Install hermes-agent[anthropic] so the provider resolves. Addresses Codex P1 (kanban_modal_worker.py:68).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23cf562076
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM. Reviewed the Modal memo-evaluator lane end-to-end against the repo's invariants: prompt-cache safety, message-role alternation, narrow-waist/footprint, .env-secrets-only, profile-safety, and correctness/security of the shim↔worker contract.
Findings against the priority lenses — all clean:
- Prompt-cache / role-alternation (#1–2): N/A. Each memo evaluation is a fresh one-shot
hermes chat -Q -qinvocation in a Modal container; nothing mutates a long-lived conversation's cached prefix or message ordering. - Narrow-waist (#3): No new core model tool. The capability is reached by extending the existing dispatcher + a
config.yamlkey — the correct low rung of the footprint ladder. .env/HERMES_ (#4):* The backend toggle lives inconfig.yaml(kanban.worker_backends), not.env. TheHERMES_MODAL_MEMO_EVALUATOR_PROFILEenv var was removed (43eca19); the remainingHERMES_KANBAN_*vars are runtime IPC to the spawned child, matching_default_spawn.- Profile-safety (#9): Resolves via
get_profile_dir("memo-evaluator")under themodal.is_local()guard — no hardcoded~/.hermes. - Security: Untrusted remote result is well-contained — reserved metadata keys (
_staged_artifacts,artifacts) are stripped beforecomplete_task, and those are the only two keys that drive host-side attach/unlink, so the bug class is fully covered. Audit metadata (modal_call_id/modal_log_url) is validated. Run-id pinning viaHERMES_KANBAN_RUN_IDcorrectly rejects a stale shim landing on a re-claimed attempt. - Verified
modal run --write-resultis a real flag (writes the str/bytes return value to a local path), so the shim↔worker file contract is valid; the stdin request pipe avoids the Windows 32,767-char argv cap.
One minor, non-blocking note: in apply_modal_result, the modal-shim audit comment is written (kanban_modal.py:102) before complete_task (kanban_modal.py:108). When a stale shim's completion is rejected on a run-id mismatch, the "Modal audit: call ..." comment still lands on the re-claimed task — harmless audit noise, but you could move the comment to after a successful completion if you want to avoid it.
0 blocking, 1 minor.
…lane The opt-in setup previously read as 'change one config value', but the modal CLI is an optional extra absent even from hermes-agent[all]; without it every run blocks transient. Document the 'uv pip install modal' + 'modal setup' prerequisite (and secret/profile assets) as an ordered setup, and make the missing-CLI RuntimeError name the fix so the block reason is actionable. Addresses Codex P1 (kanban_modal.py:215).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5828ed4ad9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Reviewed the Modal memo-evaluator lane end-to-end against the AGENTS.md invariants. The plumbing is solid: backend selection is a config.yaml key (not a HERMES_* env var), the profile resolves via get_profile_dir() (profile-safe), untrusted-result metadata stripping (_staged_artifacts/artifacts) covers the full reserved-key set that complete_task acts on, the run-id is env-pinned at spawn to reject stale completions, and the parent log fd is released. No agent-core / prompt-cache / role-alternation surface is touched. One concern:
1. (blocking — verify) The remote invocation contract is exercised only against a fake modal binary, and its central mechanism does not appear to be a real Modal feature. The shim reads the worker result from a file it asks modal run to populate via --write-result (kanban_modal.py#L224-L235), while the worker's local_entrypoint merely returns the JSON (kanban_modal_worker.py#L196-L200). modal run has no documented --write-result flag and does not persist a local-entrypoint return value to disk, so on the real path modal run would exit non-zero, _run_modal raises, and every enabled memo-evaluator run blocks as transient (kanban_modal.py:296-297) — the feature never reaches the worker. The tests only prove a hand-written fake modal that implements this invented protocol, so they can't catch this (focus #8: green mock over a live path). call.get_dashboard_url() (worker L200) is similarly unverified against the real SDK. Please drive one real modal run before enabling the lane, or switch to capturing the entrypoint's stdout, which modal run does emit.
1 blocking (pending verification against real Modal), 0 minor.
… timeout The remote evaluate_memo function was hardcoded to a 1h timeout, so a >1h or uncapped (max_runtime_seconds=None) memo task lost its Modal call at one hour and blocked as transient instead of honoring the task's runtime contract. The shim now threads max_runtime_seconds into the request; the worker's main() applies it via evaluate_memo.with_options(timeout=...), clamped to Modal's 24h ceiling, with uncapped tasks using that ceiling. Regression test proves the runtime reaches the request. Addresses Codex P2 (kanban_modal_worker.py:138). Also refutes the claude blocking concern: 'modal run -w/--write-result' is a real, documented flag (verified against modal 1.5.2) and FunctionCall.get_dashboard_url/object_id/get + Function.spawn/with_options all exist in the SDK.
|
Re: the claude-review blocking concern that
So the remote path does reach the worker; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc2fbb9f55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM overall — reviewed the Modal memo-evaluator lane against the prompt-cache, role-alternation, narrow-waist, env-var, and profile-safety invariants plus shim/worker correctness.
Verified premises:
- One-shot remote
hermes chat -Q -qper invocation — no long-lived conversation, so no prompt-cache or role-alternation exposure. - No new core model tool; backend selection routes at the dispatcher layer.
- Backend flag is a
config.yamlkey (kanban.worker_backends), not a HERMES_* env var; theHERMES_KANBAN_*vars are parent→child process IPC mirroring_default_spawn. - Profile resolution uses HERMES_HOME-aware
get_profile_dir('memo-evaluator')(rule #9 satisfied; the oldHERMES_MODAL_MEMO_EVALUATOR_PROFILEenv var was correctly dropped). - The untrusted-metadata strip targets the exact keys
complete_tasktreats as host-side directives (_staged_artifactsat kanban_db.py:5680,artifactsat 5841), so the security hardening is real and sufficient. - Spawned-run-id pinning correctly rejects stale completions on a re-claimed attempt.
One minor, non-blocking note:
Local shim timeout can preempt the remote before its own budget elapses — hermes_cli/kanban_modal.py:288-293 uses the task's max_runtime_seconds as both the remote function timeout AND the local subprocess.run(timeout=...). The remote clock starts only after cold-start/queue, so local wall-clock (overhead + queue + execution) exceeds remote execution time; a task using most of its budget hits local TimeoutExpired first, killing modal run, orphaning the paid remote call, and blocking the task as transient — partly undermining the runtime-honoring fix. Suggest giving the local subprocess headroom over the remote timeout (e.g. timeout + buffer) or leaving the local call uncapped and relying on the remote 24h ceiling.
0 blocking, 1 minor.
Two Codex findings on the memo-evaluator Modal lane: P1 (kanban_modal.py): the shim spawned `modal run` as a blocking child, but a dispatcher reclaim/timeout signals only the recorded shim pid. Killing the shim orphaned the Modal CLI and its paid remote call, which a requeued attempt could then duplicate. Launch the child in its own process group, track it, and forward SIGTERM->SIGKILL to the whole group on the shim's own signal handler and on the per-task timeout path so no orphaned remote call survives. P2 (kanban_modal_worker.py): a max_runtime_seconds above Modal's 24h function cap was silently clamped, so a longer task would be killed mid-evaluation and requeued as a transient failure forever. Reject it up front as a capability block (extracted into the pure _resolve_function_timeout helper) so a human reroutes it before any paid spawn.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae2af15701
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Reviewed the new Modal memo-evaluator lane against the repo's core invariants — no blocking issues.
Invariants checked (all clean):
- Prompt-cache / role-alternation: N/A — this is a Kanban worker-spawn path running fresh single-shot
hermes chat -Qinvocations, not the long-lived conversation loop. No mid-conversation mutation. - Narrow-waist: No new core model tool — capability is reached via CLI-invoked modules + subprocess (correct rung of the ladder).
- Config vs .env: The backend toggle is a
config.yamlkey (kanban.worker_backends), not aHERMES_*env var. TheHERMES_KANBAN_*env vars are subprocess-passing state (mirroring_default_spawn), andresearch-proxyis a real credential in a Modal secret. Compliant. - Profile-safety:
_default_profile_source()usesget_profile_dir("memo-evaluator")rather than a hardcoded~/.hermes. Good. - Removed side effects:
_configured_worker_spawndelegates to_default_spawnfor every non-modal lane, so local behavior is preserved; review-column dispatch intentionally stays local. - Untrusted-input handling: stripping
_staged_artifacts/artifactsfrom remote results, refusing attachment tasks, and pinningexpected_run_idare all solid.
Minor (non-blocking):
- Stale-run audit comment ordering — :
hermes-agent/hermes_cli/kanban_modal.py
Lines 165 to 177 in ae2af15
add_comment("Modal audit: ...")runs beforecomplete_task, which returnsFalseon anexpected_run_idmismatch. A stale shim that loses the run-id race (the scenariotest_modal_shim_pins_the_spawned_run_idexercises) still posts a "Modal audit: call X" comment onto the task now running under a different attempt, falsely implying that run completed via Modal. Fix: gate the auditadd_commentoncomplete_taskreturningTrue. test_worker_binds_the_memo_evaluator_model_and_anthropic_proxy_secretfreezesMEMO_EVALUATOR_MODEL == "claude-fable-5"— a hardcoded model literal that must be edited in lockstep whenever the profile's pinned model changes. TheMEMO_EVALUATOR_PROVIDER == "anthropic"assertion carries the real invariant (must match the Anthropic-format proxy secret); consider dropping the exact-model assertion or asserting the relationship instead.
0 blocking, 2 minor.
Codex P2 re-review: a shim spawned for a reclaimed attempt still launched (and paid for) a duplicate remote evaluation because the env-pinned run id was only compared at result-application, not before invocation. Add a pre-invocation staleness guard in run_modal_shim: when the spawned run id no longer matches the task's current run, return without calling _run_modal, leaving the current attempt untouched for its own shim to own.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91aeed8248
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM — reviewed the Modal memo-evaluator lane across all 14 commits.
Clean on the priority invariants:
- Prompt-cache / role-alternation: untouched — the remote path is a one-shot
hermes chat -Q -qper task, no long-lived conversation, no system-prompt mutation, no synthetic mid-loop turns. - Footprint: no new core model tool — routed by extending the existing dispatcher spawn path (
_configured_worker_spawn→_default_spawnfallback), the correct low rung of the ladder. - Env vars: the behavioral toggle lives in config.yaml (
kanban.worker_backends); the earlierHERMES_MODAL_MEMO_EVALUATOR_PROFILEconfig env var was dropped (commit 07). RemainingHERMES_KANBAN_*vars are runtime IPC to the child, matching_default_spawn. - Profile-safety: profile source resolves via
get_profile_dir("memo-evaluator"), not a hardcoded~/.hermes. - Security: untrusted remote metadata is stripped of reserved host-side directive keys (
_staged_artifacts,artifacts) beforecomplete_task— confirmed those are the keys that create/unlink attachments at kanban_db.py#L5678-L5689. Attachment tasks are refused, run-id staleness is guarded pre-invocation and at apply time, and themodal runchild is process-grouped so a reclaim can't orphan a paid call.
Also verified the review-column dispatch uses _default_spawn directly, so review agents are never routed to Modal.
Minor (non-blocking): test_worker_binds_the_memo_evaluator_model_and_anthropic_proxy_secret asserts MEMO_EVALUATOR_MODEL == "claude-fable-5" against the module constant — a change-detector on a model literal that breaks on a legitimate repin without catching a bug. The adjacent provider assertion carries the real contract.
0 blocking, 1 minor.
…non-dict request Two re-review findings: - codex P2 (kanban_modal.py): apply_modal_result wrote the Modal call-id/log-url audit comment before complete_task's run-id validation, so a stale shim (task reclaimed mid-call) smeared old audit onto the new attempt even though the result was rejected. Complete first; add the comment only when the run-validated completion actually landed. - graphite (kanban_modal_worker.py): main's json.loads(...).get() raised an uncaught AttributeError on valid-but-non-dict JSON (list/str/number), crashing the entrypoint. Catch AttributeError/JSONDecodeError and treat as uncapped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 592eac7bb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
LGTM — reviewed the new Modal memo-evaluator lane (kanban_modal.py, kanban_modal_worker.py), its dispatch wiring, the complete_task/artifact helpers it depends on, and the config/docs/tests.
Highlights verified:
- Config, not env (#4): backend flag lives in
config.yaml(kanban.worker_backends);HERMES_KANBAN_*are the existing dispatcher→worker IPC vars and the Modal proxy secret is a real credential. Clean. - Profile-safety (#9): profile resolved via
get_profile_dir('memo-evaluator')undermodal.is_local(), HERMES_HOME-aware. - No new core tool (#3): reached through existing dispatch + config, correct rung of the footprint ladder.
- Security: untrusted remote metadata has
_staged_artifacts/artifactsstripped beforecomplete_task; traced the prose-promotion path and confirmed it's bounded to the task's own managed scratch workspace, so no arbitrary-host-file attachment vector survives. - Run-id pinning / dup-spawn / signals: stale-shim guard enforced both pre-invocation and via
complete_task's strictcurrent_run_idguard (no snap);modal runchild is process-group-terminated on reclaim/timeout. Review-queue keeping_default_spawnis intentional (Modal's memo image can't serve thesdlc-reviewflow). - No prompt-cache / role-alternation surface — each remote eval is a fresh single-query
hermes chat -Q.
Minor (non-blocking): test_worker_binds_the_memo_evaluator_model_and_anthropic_proxy_secret asserts MEMO_EVALUATOR_MODEL == 'claude-fable-5' against the same module constant — a change-detector that freezes a model id and adds no behavioral coverage.
0 blocking, 1 minor.
…audit on run Two more re-review findings on the untrusted-remote-result boundary: - codex P1: complete_task's legacy prose scanner promotes scratch-workspace file paths named in summary/result into Kanban attachments. A prompt-injected remote Modal verdict could name a workspace secret file and expose it to later board users; the metadata-key stripping did not cover the prose route. Add a scan_prose_artifacts flag (default True for trusted local workers) and pass False from the Modal shim so remote summaries cannot drive host-file promotion. - codex P2: the shim's capability/failure handlers wrote their audit comment before block_task's run-id validation, smearing a stale attempt's note onto the re-claimed run. Block first; write the comment only when the run-validated block landed. Mirrors the completion-path audit gating.
There was a problem hiding this comment.
LGTM — reviewed the Modal memo-evaluator lane through the prompt-cache, role-alternation, narrow-waist, and env-var/config lenses, plus the untrusted-remote-result security boundary.
Invariants hold:
- No prompt-cache / role-alternation impact. The remote worker is a fresh, one-shot
hermes chat -Q -qsubprocess per task — no mid-conversation system-prompt rebuild or synthetic user turn. - Footprint respected. No new core model tool; this extends the existing dispatcher spawn path (
_configured_worker_spawn) + adds a CLI-invoked shim module. The signature-introspection back-compat at kanban_db.py#L9754-L9767 is intact and_configured_worker_spawnacceptsboard. - No new HERMES_ config env var.* Behavior flip lives in
config.yaml(kanban.worker_backends); theHERMES_KANBAN_*vars are the pre-existing dispatcher→child IPC pattern, and credentials stay in the named Modal secret. Docs correctly point users to config.yaml, not.env. - Untrusted-result handling is solid. Reserved metadata keys (
_staged_artifacts/artifacts) are stripped andscan_prose_artifacts=Falsecloses the prose-promotion route — I confirmed both are the only host-file-touching directive paths incomplete_task(kanban_db.py#L5690-L5701). Run-id gating (pre-invocation skip + audit-comment-only-on-landed-completion) correctly prevents stale-shim duplicate billing and audit smearing. - Profile-safe.
_default_profile_sourceresolves viaget_profile_dir(), honoring HERMES_HOME.
Minor (non-blocking): MEMO_EVALUATOR_MODEL = "claude-fable-5" is hardcoded in kanban_modal_worker.py#L34-L37 rather than read from the memo-evaluator profile's config.yaml. Since the local_entrypoint main() runs locally with hermes_cli available, it could resolve the profile's configured model and thread it through the request — otherwise an operator who repoints their profile to a different model gets silent drift on the remote lane. The paired test freezes the literal, so drift wouldn't surface.
0 blocking, 1 minor.
1 participant
Summary
memo-evaluatorKanban laneSOUL.mdand skills into the Modal image; proxy credentials stay in the named Modal secretVerification
scripts/run_tests.sh tests/hermes_cli/test_kanban_modal.py tests/hermes_cli/test_kanban_worker_spawn_toolsets.py tests/hermes_cli/test_kanban_dispatch_lock.py -j 3scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py -j 4scripts/run_tests.sh tests/hermes_cli/test_kanban_core_functionality.py -j 4ruff checkon changed Python filespython3 -m hermes_cli.kanban_modal --helpplus missing-argument probeNotes
modal run --write-resultprotocol is exercised with a fake CLI in the focused tests.