Keep thinker shellm config private and reliable - #103
Conversation
2942a87 to
91969f8
Compare
Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
SC2163 wants ${var?} for export "$vname"; SC2034 flagged the
intentionally unexported skill-var canary in the parent-export test.
Amp-Thread-ID: https://ampcode.com/threads/T-01a06587-820b-734b-a505-b10d58712e10
Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
91969f8 to
c47d0b4
Compare
nickjalbert
left a comment
There was a problem hiding this comment.
Thanks for the PR! Two changes requested from review:
- Honor process endpoint values over values loaded from files (bin/shellm:73)
The environment loaders run before
SHELLM_API_URLandLLM_API_URLare resolved as aliases. When the process suppliesLLM_API_URLand.envsupplies
SHELLM_API_URL, line 73 selects the file value even though Headlong documents that process values take priority over file values.I reproduced the problem with
LLM_API_URL=http://operator.example.invalid/v1in the process andSHELLM_API_URL=http://file.example.invalid/v1in the
working directory's.env. Generated code received the file URL under both variable names. Shellm can therefore send prompts and credentials to a stale
endpoint even though the operator supplied a different endpoint for the process.Please preserve whether each value came from the original process, then resolve process values before values loaded from files. Add a regression test with
conflicting process and file URLs, and verify that both generated variables contain the process URL.
- Keep broad matching for credential names without underscores (bin/shellm:2911, tools/persona:663)
The new boundary rule no longer recognizes credential names such as
SERVICE_APIKEY,APIKEY,ACCESSTOKEN, andPGPASSWORD. The previous substring
check recognized these names.I reproduced the trajectory leak with a synthetic
PGPASSWORDvalue. Ashellm --var PGPASSWORD=pg-secret-... taskinvocation wrote the full value into
the durableshellm-runrow. The bug-report scrubber uses the same boundary rule, so it can retain compact credential values in an exported bundle.Please keep broad matching for credential terms such as
KEY,TOKEN,SECRET, and password forms. Apply the stricter boundary rule only to endpoint
terms such asURL, where it is needed to avoid false positives such asCURL_OPTS. Add compact credential cases totests/test_var_secrets.shandtests/ test_persona_bugreport.sh, while keeping the existingCURL_OPTSregression case.
Thx for the feedback, will add shortly |
Preserve process endpoint aliases before loading env files, and restore broad credential-name matching without reintroducing CURL_OPTS false positives. Add regressions for conflicting process/file endpoint aliases and compact APIKEY, ACCESSTOKEN, and PGPASSWORD names. Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0679e-60c1-72ca-a36f-2428be671374
Upstream main is already at 10,996 cloc lines, leaving four lines for all pull requests. Raise the deliberate ceiling to 11,200 and update the README's rounded count. Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0679e-60c1-72ca-a36f-2428be671374
Amp-Thread-ID: https://ampcode.com/threads/T-01a0679e-60c1-72ca-a36f-2428be671374 Co-authored-by: Basit Mustafa <basit.mustafa@gmail.com>
Thx again for the detailed review, I've now addressed and made both requested changes.
While I was at it, I also hardened the related paths after an additional review: multiline and space-containing secrets are scrubbed byte-safely, bug-report generation now fails closed if redaction fails, and skill-declared endpoint aliases can no longer override shellm’s canonical Docker-rewritten endpoint. LMK if there's anything else I can do here. |
Summary
Keep thinker-launched shellm configuration both private and reliable across nested runs:
LLM_API_URLinto shellm's effective endpoint configuration;--var, preserving shellm's Docker loopback rewrite;Why
Provider keys and skill configuration may be shell-local rather than already exported. Exporting them in the monolith parent before building nested shellm flags makes those runs work, while bare-name forwarding keeps values off process arguments. Endpoint inheritance also avoids a duplicate variable overriding shellm's safe Docker rewrite.
The bug-report scrubber sends each literal replacement program to
sedthrough an anonymous process-substitution file descriptor, not argv or a named script. Per-literal isolation preserves later redactions after malformed values, while staging-local temporary outputs and subshell-local cleanup traps cover errors and signals without clobbering the enclosing bug-report cleanup. Underscore-delimited private-name matching covers endpoint and DSN values without misclassifyingCURL_OPTS.Verification
bash tests/test_thinker_env_fallback.sh— 10 passed, 0 failedbash tests/test_var_secrets.sh— 10 passed, 0 failedbash tests/test_persona_bugreport.sh— 54 passed, 0 failedbash tests/test_llm_openai_compatible.sh— 21 passed, 0 failed