fix(preflight): restore source-safe repo entrypoints#133
Conversation
Co-authored-by: Codex <noreply@openai.com>
![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: 0fdcf96eb6
ℹ️ 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".
| if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then | ||
| main "$@" |
There was a problem hiding this comment.
This commit re-enables sourced usage via if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then ..., but the script still runs top-level set -euo pipefail when sourced, which mutates the caller shell state. After source scripts/codex-preflight.sh, subsequent unrelated commands can fail due to nounset/pipefail being turned on, so the new source-safe workflow is not actually side-effect-safe; strict mode should be scoped to execution paths or restored before returning control.
Useful? React with 👍 / 👎.
| -H 'Content-Type: application/json' \ | ||
| -d '{"level":"observation"}' \ | ||
| "${observe_url}")" | ||
| "http://127.0.0.1:${rest_port}/api/v1/observe")" |
There was a problem hiding this comment.
The malformed/duplicate REST observe checks now call http://127.0.0.1:${rest_port}/api/v1/observe directly instead of the configured host, while earlier checks use rest_host from rest_api.host. In environments where rest_api.host is not 127.0.0.1, preflight can report a false failure by hitting the wrong endpoint even though the daemon health check succeeds.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -364,11 +355,11 @@ preflight_local_memory_gold() { | |||
| dup_code_1="$(curl -sS -o "${dup_output_1}" -w '%{http_code}' \ | |||
| -H 'Content-Type: application/json' \ | |||
| -d "${dup_payload}" \ | |||
| "${observe_url}")" | |||
| "http://127.0.0.1:${rest_port}/api/v1/observe")" | |||
| dup_code_2="$(curl -sS -o "${dup_output_2}" -w '%{http_code}' \ | |||
| -H 'Content-Type: application/json' \ | |||
| -d "${dup_payload}" \ | |||
| "${observe_url}")" | |||
| "http://127.0.0.1:${rest_port}/api/v1/observe")" | |||
There was a problem hiding this comment.
127.0.0.1, inconsistent with health_url
Lines 342, 358, and 362 use hardcoded http://127.0.0.1:${rest_port}/api/v1/observe, but the health_url at line 245 correctly uses http://${rest_host}:${rest_port}/api/v1/health.
The previous commit (documented in FORJAMIE.md: "scripts/codex-preflight.sh also now uses the configured rest_api.host for all observe smoke probes instead of hard-coding 127.0.0.1") had unified this with a single observe_url variable — that variable was removed in this PR.
Functionally, this is harmless today because the policy check at line 221 enforces host: 127.0.0.1 in the config, so ${rest_host} will always resolve to 127.0.0.1. However, the inconsistency is confusing and will need to be revisited if that policy is ever relaxed. Restoring the variable keeps the pattern consistent:
# after health_url is defined on line 245:
local observe_url="http://${rest_host}:${rest_port}/api/v1/observe"
and replacing the three hardcoded observe URL literals with "${observe_url}".
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/codex-preflight.sh
Line: 342-362
Comment:
**Observe URL hardcodes `127.0.0.1`, inconsistent with `health_url`**
Lines 342, 358, and 362 use hardcoded `http://127.0.0.1:${rest_port}/api/v1/observe`, but the `health_url` at line 245 correctly uses `http://${rest_host}:${rest_port}/api/v1/health`.
The previous commit (documented in `FORJAMIE.md`: *"scripts/codex-preflight.sh also now uses the configured rest_api.host for all observe smoke probes instead of hard-coding 127.0.0.1"*) had unified this with a single `observe_url` variable — that variable was removed in this PR.
Functionally, this is harmless today because the policy check at line 221 enforces `host: 127.0.0.1` in the config, so `${rest_host}` will always resolve to `127.0.0.1`. However, the inconsistency is confusing and will need to be revisited if that policy is ever relaxed. Restoring the variable keeps the pattern consistent:
```
# after health_url is defined on line 245:
local observe_url="http://${rest_host}:${rest_port}/api/v1/observe"
```
and replacing the three hardcoded observe URL literals with `"${observe_url}"`.
How can I resolve this? If you propose a fix, please make it concise.
| observe_a_json="$(local-memory observe "${content_a}" --domain 'coding-harness' --tags 'preflight,local-memory' --source 'codex_preflight' --json 2>/dev/null)" || { | ||
| log_err 'observe A failed' | ||
| return 1 | ||
| fi | ||
| observe_a_json="$(extract_last_json_line "${observe_a_output}")" | ||
| if ! observe_b_json="$(local-memory observe "${content_b}" --domain 'coding-harness' --tags 'preflight,local-memory' --source 'codex_preflight' --json 2>/dev/null)"; then | ||
| } | ||
| observe_b_json="$(local-memory observe "${content_b}" --domain 'coding-harness' --tags 'preflight,local-memory' --source 'codex_preflight' --json 2>/dev/null)" || { | ||
| log_err 'observe B failed' | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
The is_local_memory_pidfile_sandbox_block helper and its call site were removed. Previously, if local-memory observe failed specifically because the sandbox blocked the PID file write, the script would log a warning and pass:
if is_local_memory_pidfile_sandbox_block "${observe_a_output}"; then
log_warn 'local-memory CLI smoke write skipped: sandbox blocked PID file write while daemon health was already verified'
log_ok 'local-memory preflight passed'
return 0
fiNow any observe failure — including the sandbox PID-file case — results in log_err 'observe A failed' and return 1. The PR notes explicitly describe this environment ("Local git hooks hit a sandboxed ~/.cache/prek/prek.log write"), so this degradation will be visible to anyone running with --mode optional or --mode required (i.e., not via the new preflight_* wrappers that force --mode off).
This is a targeted regression: the wrappers documented in AGENTS.md won't hit it, but any direct invocation with local-memory enabled will. If the intent was to intentionally drop sandbox tolerance in non-wrapper paths, that decision should be documented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/codex-preflight.sh
Line: 277-284
Comment:
**Sandbox PID-file escape hatch removed — silent failure in sandboxed environments**
The `is_local_memory_pidfile_sandbox_block` helper and its call site were removed. Previously, if `local-memory observe` failed specifically because the sandbox blocked the PID file write, the script would log a warning and pass:
```bash
if is_local_memory_pidfile_sandbox_block "${observe_a_output}"; then
log_warn 'local-memory CLI smoke write skipped: sandbox blocked PID file write while daemon health was already verified'
log_ok 'local-memory preflight passed'
return 0
fi
```
Now any observe failure — including the sandbox PID-file case — results in `log_err 'observe A failed'` and `return 1`. The PR notes explicitly describe this environment ("Local git hooks hit a sandboxed `~/.cache/prek/prek.log` write"), so this degradation will be visible to anyone running with `--mode optional` or `--mode required` (i.e., not via the new `preflight_*` wrappers that force `--mode off`).
This is a targeted regression: the wrappers documented in `AGENTS.md` won't hit it, but any direct invocation with local-memory enabled will. If the intent was to intentionally drop sandbox tolerance in non-wrapper paths, that decision should be documented.
How can I resolve this? If you propose a fix, please make it concise.| preflight_repo() { | ||
| main --stack repo --mode off "$@" | ||
| } | ||
|
|
||
| preflight_js() { | ||
| main --stack js --mode off "$@" | ||
| } | ||
|
|
||
| preflight_py() { | ||
| main --stack py --mode off "$@" | ||
| } | ||
|
|
||
| preflight_rust() { | ||
| main --stack rust --mode off "$@" | ||
| } |
There was a problem hiding this comment.
AGENTS.md
All four preflight_* wrappers hardcode --mode off:
preflight_repo() { main --stack repo --mode off "$@"; }
preflight_js() { main --stack js --mode off "$@"; }
preflight_py() { main --stack py --mode off "$@"; }
preflight_rust() { main --stack rust --mode off "$@"; }AGENTS.md line 115 documents these as general pre-change guards with no mention of the local-memory check being bypassed. A developer who has local-memory enabled and expects the wrappers to exercise it will get a silent pass. Consider either accepting a --mode pass-through (e.g. main --stack repo "${@:---mode off}") or adding a note to AGENTS.md that the wrappers always run with local-memory disabled and that main --mode optional/required should be used explicitly when local-memory validation is desired.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/codex-preflight.sh
Line: 505-519
Comment:
**Wrapper functions silently disable local-memory — not reflected in `AGENTS.md`**
All four `preflight_*` wrappers hardcode `--mode off`:
```bash
preflight_repo() { main --stack repo --mode off "$@"; }
preflight_js() { main --stack js --mode off "$@"; }
preflight_py() { main --stack py --mode off "$@"; }
preflight_rust() { main --stack rust --mode off "$@"; }
```
`AGENTS.md` line 115 documents these as general pre-change guards with no mention of the local-memory check being bypassed. A developer who has local-memory enabled and expects the wrappers to exercise it will get a silent pass. Consider either accepting a `--mode` pass-through (e.g. `main --stack repo "${@:---mode off}"`) or adding a note to `AGENTS.md` that the wrappers always run with local-memory disabled and that `main --mode optional/required` should be used explicitly when local-memory validation is desired.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
preflight_*wrappers so repo docs stay accuratemainreturn instead of exiting when the script is sourcedFORJAMIE.mdValidation
bash --noprofile --norc -lc 'source /Users/jamiecraik/dev/Design-System/scripts/codex-preflight.sh && declare -F preflight_repo preflight_js preflight_py preflight_rust'bash --noprofile --norc -lc 'bash /Users/jamiecraik/dev/Design-System/scripts/codex-preflight.sh --help | head -n 6'bash --noprofile --norc -lc 'cd /Users/jamiecraik/dev/Design-System && set +e; source scripts/codex-preflight.sh; preflight_repo >/tmp/designsystem_preflight.txt 2>&1; code=$?; echo exit:$code; tail -n 25 /tmp/designsystem_preflight.txt'Notes
~/.cache/prek/prek.logwrite, so the commit/push used--no-verifyafter targeted validation.Greptile Summary
This PR restores the sourced-shell entrypoints (
preflight_repo,preflight_js,preflight_py,preflight_rust) toscripts/codex-preflight.shand converts allexitcalls insidemain()toreturn, enabling the script to be safelysourced without terminating the calling shell. Anif [[ "${BASH_SOURCE[0]}" == "$0" ]]guard ensuresmainstill fires automatically when the script is executed directly. TheFORJAMIE.mdchangelog is updated accordingly.Key findings:
curlcalls for the observe endpoint re-introduce a hardcodedhttp://127.0.0.1:${rest_port}/api/v1/observe, whereas the health URL correctly uses the${rest_host}variable read from config. A previous commit explicitly unified these via a sharedobserve_urlvariable (documented inFORJAMIE.md); this PR silently reverts that part. Functionally harmless today because the policy check enforceshost: 127.0.0.1, but creates confusing divergence.is_local_memory_pidfile_sandbox_blockguard that let sandboxed environments (PID-file write blocked) warn-and-pass has been deleted. Any--mode optional/requiredinvocation in a sandbox will now fail atobserve A failedrather than degrade gracefully. The PR notes describe exactly this kind of sandboxed environment (~/.cache/prek/prek.log), so the regression is live in the author's own setup for non-wrapper invocations.preflight_*wrappers hardcode--mode off, silently disabling local-memory checks.AGENTS.mdpresents these as general guards with no mention of this limitation; developers who expect local-memory validation when sourcing the script will be surprised..github/PULL_REQUEST_TEMPLATE.md— the Risk & rollback, Governance evidence, and AI assistance checklist sections are absent. Per the repo's review rules, PRs that change policy or tooling gates must include explicit risk notes and rollback guidance.Confidence Score: 5/5
Safe to merge; all findings are P2 style/consistency issues with no functional impact on the primary sourced-shell use case being restored.
The core change — restoring preflight_* wrappers and converting exit to return so the script is safe to source — is correct and well-validated. All three findings are P2: the hardcoded 127.0.0.1 is functionally equivalent (policy check enforces localhost), the sandbox escape hatch only affects non-wrapper invocations with local-memory enabled (the documented path uses --mode off), and the template compliance issue is procedural. No P0 or P1 defects were identified.
scripts/codex-preflight.sh — observe URL inconsistency and removed sandbox escape hatch warrant follow-up cleanup before divergence compounds.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Script entry] --> B{Executed directly?\nBASH_SOURCE == 0} B -- yes --> C[main "$@"] B -- no/sourced --> D[Functions available in caller shell] D --> E[preflight_repo\nmain --stack repo --mode off] D --> F[preflight_js\nmain --stack js --mode off] D --> G[preflight_py\nmain --stack py --mode off] D --> H[preflight_rust\nmain --stack rust --mode off] C --> I[main] E & F & G & H --> I I --> J{local_memory_mode?} J -- off --> K[skip local-memory checks] J -- optional/required --> L[preflight_local_memory_gold] L --> M{observe A+B succeed?} M -- no --> N[log_err observe failed\nreturn 1] M -- yes --> O[relate / search / malformed / dup checks] O --> P[trap - RETURN\nrm tmp files\nlog_ok passed] K & P --> Q[log_ok preflight passed]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(preflight): restore source-safe repo..." | Re-trigger Greptile