refactor doc#7
Closed
gogo2464 wants to merge 2 commits into
Closed
Conversation
|
test seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
|
What's the purpose of these changes? I thought you were working on migrating the user manual into this codebase. |
Author
|
@sanity : I read the user manual as starting. It has took me a few times. I think the actual documentation of freenet is VERY confusing. I am expert in clarifying documentation. I realized that the doc of freenet is very confusing about the I split the doc in 4 types:
I want to make documentation navigation easier for everyone reading the web site. I am definitely sorry for the fact you dislike. I am supposed to shall make freenet dev easier and faster. Not harder and slower. I think it could be a great feature. Check out https://docs.divio.com/documentation-system/ for details. Best regards. |
Author
|
@sanity I see the lack of your interest in my way to solve it. I suggest that I: I should have been more organized and more incremental!! |
Contributor
|
Closing this. We had agreed on a straightforward task: migrating the user manual markdown pages to the website. Instead, you significantly changed the site’s structure and design. That was over a month ago, and the original task still isn’t done. If you're not able to stay focused on the scope we agree on, then I don’t have the bandwidth to keep managing this. |
Author
|
Yes sure. Close. I will redo it better. |
sanity
added a commit
that referenced
this pull request
Apr 12, 2026
Batch of fixes from the four parallel review agents + codex CLI on PR #26. See PR comments for the full review threads. Version bump: 0.1.5 → 0.2.0 (codex P2 / breaking) ============================================ The rename changes public struct fields (`GhostkeyCertificateV1.delegate` → `.notary`, `DelegatePayload.delegate_verifying_key` → `.notary_verifying_key`). Under pre-1.0 Rust semver, 0.1.x → 0.1.y is the *compatible* range, so Cargo would auto-upgrade downstream crates and break their builds without opt-in. 0.2.0 is the *breaking* range and requires explicit opt-in, which is the correct semver answer for a field rename. CHANGELOG.md, README.md, and all "removed in 0.2.0" deprecation notices rewritten to reflect the new target. API --notary-dir env-fallback fix (skeptical HIGH #1) ===================================================== `rust/api/src/main.rs` previously declared --notary-dir as required(true) with no .env() binding. Operators who had only DELEGATE_DIR set in systemd units or .env files would hit a hard clap error at startup; the DELEGATE_DIR fallback path in `delegates::notary_dir` was unreachable from the binary. Fix: - Pre-parse hook in main() copies DELEGATE_DIR into NOTARY_DIR early (with a deprecation warning) if NOTARY_DIR is unset. - --notary-dir is now bound to .env("NOTARY_DIR"), so either the CLI flag or the env var satisfies the required constraint. - Removed the now-unused LEGACY_DELEGATE_DIR static and the double-set env::set_var at startup. API per-amount pair-atomicity fix (skeptical MEDIUM #3) ======================================================= `rust/api/src/delegates.rs::resolve_amount_file` used to resolve the cert and signing-key paths independently. If an operator mid-migration had renamed the cert but not the signing key (or vice versa), the API would happily pair a new-named cert with a legacy-named signing key for the wrong amount — every ghost cert issued for that amount would have an invalid signature chain. Replaced with `pick_scheme()`, which resolves the naming scheme at the directory level, per amount. If a complete `notary_*` pair exists it's used; else if a complete `delegate_*` pair exists it's used with a deprecation warning; else it defaults to `notary_*` so the "not found" error points at the canonical filename. Six unit tests pin every resolution branch including the two partial-migration cases. Codex wrong-repository-URL fix (LOW #7) ======================================= `rust/gklib/Cargo.toml` listed `repository = "https://github.com/freenet/freenet-core"` which is wrong — the crate lives in `freenet/web`. Pre-existing but this PR publishes it, so fix in the same commit. Corrected to `https://github.com/freenet/web`. Testing reviewer HIGH #1 — unit tests for fallback resolvers ============================================================ Added `#[cfg(test)] mod resolve_tests` to `rust/cli/src/commands.rs` covering `resolve_notary_file` (4 tests: prefer canonical, fall back to legacy, not-found returns canonical, signing key variant) and `delegates::tests` in `rust/api/src/delegates.rs` (6 tests: prefer notary, fall back to legacy, refuse partial-notary pair, refuse partial-legacy pair, missing directory, filename format contract). Terminology: "wallet" → "vault" (product-name accuracy) ======================================================== Issue #24 referred to the Ghostkeys delegate as a "wallet delegate" but the actual product name in `freenet/ghostkeys/ui/` is "Ghostkey Vault" / "Identity Vault" (class names `vault-section`, `empty-vault`, page title "Ghostkey Vault"). Updated the gklib AGENTS.md terminology section and the CHANGELOG to call out three distinct concepts: - notary certificate (PKI intermediate, this crate) - Freenet `Delegate` (generic platform primitive, sandboxed WASM agent) - Ghostkey Vault (specific freenet/ghostkeys delegate, user-facing product name) None of my committed files ever said "wallet" in prose, but the AGENTS.md section was previously silent on the vault product name; now it specifically warns readers not to parrot issue #24's language. Miscellaneous cleanups from code-first + testing reviewers ========================================================== - `rust/integration_test/src/browser_test.rs:220` error-message string used "Delegate certificate validation failed" — now accepts both the legacy and the new error text so migrations work either way. - `rust/integration_test/src/environment.rs:29` set `GHOSTKEY_DELEGATE_KEY_DIR` alongside `NOTARY_DIR`, but nothing in the repo reads that env var — dead code from the pre-rename state, removed. - Updated `rust/gklib/src/delegate_certificate.rs` #[deprecated] `since` attributes from "0.1.5" to "0.2.0" to match the actual release. - Fixed inline "0.1.5" references in source doc comments to match the 0.2.0 target. Test results (all green locally) ================================ - `cargo test --manifest-path rust/gklib/Cargo.toml --all-features`: 22 unit + 6 legacy fixture compat tests pass - `cargo test --manifest-path rust/Cargo.toml --all-features`: 14 ghostkey-api tests pass (includes the 6 new pick_scheme tests), 3 gkwasm tests pass, integration_test builds - `cargo test --manifest-path rust/cli/Cargo.toml`: 4 new resolve_notary_file tests pass - `test_cli_commands.sh`: 17/17 green (both new + legacy spellings) Refs #24 [AI-assisted - Claude]
sanity
added a commit
that referenced
this pull request
Apr 12, 2026
* refactor(workspace): switch api + integration_test to path dep on gklib Before the rename in #24, make the workspace compile coherently against in-tree gklib. rust/api pinned registry ghostkey_lib = 0.1.3 and rust/integration_test pinned 0.1.0; neither would pick up source changes to rust/gklib until a new release was published to crates.io. Switch both to path = "../gklib" permanently so renames in the same PR can be validated end-to-end. The published-crate drift check this removes is now a manual release-checklist item (see CHANGELOG in later commit). Refs #24 [AI-assisted - Claude] * refactor(gklib): rename delegate → notary certificate types Core of the ghost-key PKI rename from #24. The PKI intermediate signing key was historically called "delegate certificate"; this collides with Freenet's own Delegate (WASM agent) concept. Rename to "notary" to match the role (witnesses a donation and attests to it), freeing "delegate" for its intended meaning. Changes: - New module rust/gklib/src/notary_certificate.rs containing NotaryCertificateV1 and NotaryPayload (formerly DelegateCertificateV1 and DelegatePayload). - Old module rust/gklib/src/delegate_certificate.rs becomes a deprecated stub re-exporter so downstream code using the path ghostkey_lib::delegate_certificate::DelegateCertificateV1 keeps compiling with a deprecation warning. Plan is to remove in 0.2.0. - GhostkeyCertificateV1.delegate field renamed to `notary` but its CBOR wire-format key is frozen as "delegate" via #[serde(rename = "delegate")]. Same treatment for NotaryPayload.notary_verifying_key (frozen as "delegate_verifying_key"). Because cert signatures cover the serialized payload bytes, any change to the CBOR key names would invalidate every existing ghost key in the wild; bumping to a V2 format was rejected in #24 as unnecessary churn. - armorable.rs gains legacy_armor_aliases() so new code can still read PEM files with BEGIN DELEGATE_CERTIFICATE_V1 headers. Writes use the canonical BEGIN NOTARY_CERTIFICATE_V1. Concentrated in armorable.rs because the blanket Armorable impl prevents per-type trait overrides. - gkwasm and cli field accesses updated to the new Rust field names (wire format unchanged). Broader identifier renames in those crates land in later commits. All 22 existing gklib unit tests pass. Fixture-based regression tests against pre-rename cert bytes land in a later commit. Refs #24 [AI-assisted - Claude] * test(gklib): add legacy v1 compat tests for delegate → notary rename Fixtures generated from an unmodified checkout of main before the rename commits, pinned into rust/gklib/tests/fixtures/legacy_v1/: - master_verifying_key.pem - delegate_certificate.pem (header: BEGIN DELEGATE_CERTIFICATE_V1) - ghost_key_certificate.pem - signed_message.bin (PEM-armored SignedMessage) The tests in rust/gklib/tests/legacy_v1_compat.rs validate: 1. NotaryCertificateV1::from_file accepts the legacy PEM header via the armorable.rs alias table. 2. GhostkeyCertificateV1::from_file parses and verifies back to the fixture master key — proves #[serde(rename = "delegate")] on the enclosing field keeps the CBOR wire format stable. 3. The legacy SignedMessage still deserializes under new types and the ed25519 signature over the message still verifies. 4. Re-serializing a loaded legacy cert produces byte-identical CBOR, the strongest possible check that #[serde(rename = "delegate_verifying_key")] on NotaryPayload freezes the payload bytes. Any drift would invalidate the master signature. 5. New code writes the canonical BEGIN NOTARY_CERTIFICATE_V1 header and round-trips through the same parser. These tests are the line of defense against silently breaking ghost keys in the wild. If any of the serde rename attrs or the armor alias table are removed, these tests MUST fail. Refs #24 [AI-assisted - Claude] * refactor(gkwasm): rename delegate → notary in Rust identifiers Internal rename only. The wasm_bindgen-exposed function names (wasm_generate_keypair_and_blind, wasm_generate_ghost_key_certificate) are unchanged because they don't contain "delegate" and renaming them would force JS-side coordination for no naming-clarity win. Parameter names on the wasm_bindgen functions are JS-positional, not JS-named, so renaming them in Rust has no effect on existing JS callers. donation-success.js continues to pass arguments positionally. Refs #24 [AI-assisted - Claude] * refactor(cli): rename delegate → notary subcommands, flags, and files - New canonical subcommands: generate-notary, verify-notary - Legacy generate-delegate / verify-delegate retained as clap aliases - New canonical flags: --notary-certificate, --notary-dir - Legacy --delegate-certificate / --delegate-dir retained as aliases - Default on-disk filenames: notary_certificate.pem, notary_signing_key.pem (was delegate_*.pem) - generate-ghost-key reads the new canonical filenames first and falls back to the legacy delegate_*.pem names with a deprecation warning, so existing key directories keep working unchanged - commands.rs identifier rename (generate_delegate_cmd → generate_notary_cmd, etc.) plus the resolve_notary_file() helper that encapsulates the fallback read pattern - warn_on_legacy_argv() in main() pre-scans argv for the legacy subcommand and flag spellings and emits a one-shot stderr deprecation warning before clap parses. Clap's Command::alias and Arg::alias normalize matches to the canonical name, so a post-parse check cannot distinguish which spelling the user typed. Manually verified both spellings round-trip end-to-end: generating keys with `generate-delegate` produces the new filenames; running `generate-ghost-key --delegate-dir` against a directory with legacy delegate_*.pem files warns and reads them correctly. Scripts (test_cli_commands.sh, test_ghostkey.sh, generate_delegate_keys.sh) and the rust/api DELEGATE_DIR env var land in the next commit. Refs #24 [AI-assisted - Claude] * refactor(api+scripts): rename delegate → notary on-disk, env, scripts End-to-end rename of the delegate → notary surface outside of gklib: rust/api/src/delegates.rs (now about notary keys despite filename): - get_notary()/sign_with_notary_key() replace get_delegate()/sign_with_delegate_key() - notary_dir() reads NOTARY_DIR first, falls back to DELEGATE_DIR with a deprecation warning - resolve_amount_file() reads notary_{certificate,signing_key}_{amount}.pem first, falls back to delegate_*_{amount}.pem with a deprecation warning - Module filename (delegates.rs) kept for minimal churn — doc comment explains the rename rust/api/src/routes.rs: - DonationResponse dual-emits BOTH delegate_certificate_base64 (legacy) and notary_certificate_base64 (canonical) fields with identical values for one release. Lets cached browser JS keep working while freshly served JS picks up the new field. rust/api/src/handle_sign_cert.rs: - SignCertificateResponse dual-emits the same way rust/api/src/main.rs: - --notary-dir flag canonical, --delegate-dir clap alias - Pre-scan argv for --delegate-dir and emit stderr deprecation warning - env vars NOTARY_DIR + DELEGATE_DIR both set on startup for maximal downstream compat rust/cli/generate_notary_keys.sh: - New canonical script. Writes notary_{certificate,signing_key}_{amount}.pem. - Uses `ghostkey generate-notary` and --notary-dir. - Accepts --delegate-dir as a legacy alias with a deprecation warning. - Inline comment pinning the "delegate-key-created" JSON key inside the cert info field — this is baked into every historical donation cert and must NOT be renamed. rust/cli/generate_delegate_keys.sh: - Becomes a one-line stub that warns and execs generate_notary_keys.sh rust/integration_test: - setup_delegate_keys → setup_notary_keys, generate_delegate_keys → generate_notary_keys; invokes generate_notary_keys.sh with --notary-dir; sets both NOTARY_DIR and the legacy env var - services.rs passes --notary-dir to the API test_cli_commands.sh + rust/cli/test_ghostkey.sh: - Exercise BOTH the new notary spelling and the legacy delegate aliases to lock in backward compatibility - Fixed a pre-existing `set -e` + `((x++))` footgun in test_cli_commands.sh (post-increment returns the old value, so the counter incrementing from 0 returned 1 and killed the script before any test ran). Now the script actually runs all 17 tests. Makefile.toml: - generate-notary-keys canonical, generate-delegate-keys as a dependent alias Verified end to end: - test_cli_commands.sh: 17/17 pass - test_ghostkey.sh: 19/19 pass - cargo check --manifest-path rust/Cargo.toml clean Refs #24 [AI-assisted - Claude] * docs(web): propagate delegate → notary rename through JS, site, docs hugo-site/themes/freenet/layouts/shortcodes/stripe-donation-form.html: - Read notary_certificate_base64 from API response first, fall back to legacy delegate_certificate_base64 for older servers - Write the cert to BOTH localStorage keys so already-cached donation-success.js (which reads the legacy key) keeps working during the transition. Removal tracked in #24 for 0.2.0. hugo-site/static/js/donation-success.js: - Read notary_certificate_base64 first; fall back to delegate_certificate_base64 and migrate the value to the new key. Do NOT delete the legacy key for one release — prevents a stale multi-tab session from losing its cert. - Pass the cert positionally to wasm_generate_keypair_and_blind and wasm_generate_ghost_key_certificate; rename the local var. hugo-site/content/ghostkey/_index.md: - One sentence referring to the PKI intermediate as "delegate key" now says "notary key". Other occurrences on the page refer to the Freenet WASM Delegate (wallet agent) and are correctly left alone. - Deliberately NOT touching hugo-site/content/about/news/introducing-ghost-keys.md — that's a historical blog post and the embedded JSON key "delegate-key-created" is baked into actual cert info fields. rust/gklib/README.md + rust/gklib/AGENTS.md: - Update type names, module paths, CLI commands, and trust-chain wording. Add a dedicated "notary vs delegate" terminology callout. - New wire-format compatibility section in README explaining the serde rename + armor alias strategy. rust/cli/README.md: - Subcommand list, features, and narrative updated to the notary spelling with a note that legacy aliases are still accepted. Refs #24 [AI-assisted - Claude] * chore(gklib): bump to 0.1.5 + CHANGELOG for delegate → notary rename Final commit of the PR A rename work (issue #24). Bumps ghostkey_lib from 0.1.4 to 0.1.5 (patch — this is a source-API rename with deprecated type aliases, so downstream code using the old names compiles with warnings). CHANGELOG.md documents every rename, every compatibility shim, the wire-format freeze rationale, what was deliberately NOT renamed and why, upgrade guidance, and a release-checklist addendum: after publishing to crates.io, temporarily flip rust/api/Cargo.toml back to registry ghostkey_lib = "0.1.5" and `cargo check` to validate the published crate doesn't drift from in-tree source (replaces the automatic check lost when api and integration_test moved to path deps in commit 1). Refs #24 [AI-assisted - Claude] * style: cargo fmt across files touched by the rename Apply `cargo fmt` to files modified during the rename (gklib/, cli/, api/, gkwasm/, integration_test/). These are the files I already touched — rustfmt tidies them up so a diff viewer won't show mixed-formatting hunks. Files NOT touched by the rename (rate_limit.rs, invite.rs, browser_test.rs, cli.rs, errors.rs) are intentionally left alone to keep the PR diff focused. Refs #24 [AI-assisted - Claude] * fix(gklib): bump to 0.2.0 + address PR review findings Batch of fixes from the four parallel review agents + codex CLI on PR #26. See PR comments for the full review threads. Version bump: 0.1.5 → 0.2.0 (codex P2 / breaking) ============================================ The rename changes public struct fields (`GhostkeyCertificateV1.delegate` → `.notary`, `DelegatePayload.delegate_verifying_key` → `.notary_verifying_key`). Under pre-1.0 Rust semver, 0.1.x → 0.1.y is the *compatible* range, so Cargo would auto-upgrade downstream crates and break their builds without opt-in. 0.2.0 is the *breaking* range and requires explicit opt-in, which is the correct semver answer for a field rename. CHANGELOG.md, README.md, and all "removed in 0.2.0" deprecation notices rewritten to reflect the new target. API --notary-dir env-fallback fix (skeptical HIGH #1) ===================================================== `rust/api/src/main.rs` previously declared --notary-dir as required(true) with no .env() binding. Operators who had only DELEGATE_DIR set in systemd units or .env files would hit a hard clap error at startup; the DELEGATE_DIR fallback path in `delegates::notary_dir` was unreachable from the binary. Fix: - Pre-parse hook in main() copies DELEGATE_DIR into NOTARY_DIR early (with a deprecation warning) if NOTARY_DIR is unset. - --notary-dir is now bound to .env("NOTARY_DIR"), so either the CLI flag or the env var satisfies the required constraint. - Removed the now-unused LEGACY_DELEGATE_DIR static and the double-set env::set_var at startup. API per-amount pair-atomicity fix (skeptical MEDIUM #3) ======================================================= `rust/api/src/delegates.rs::resolve_amount_file` used to resolve the cert and signing-key paths independently. If an operator mid-migration had renamed the cert but not the signing key (or vice versa), the API would happily pair a new-named cert with a legacy-named signing key for the wrong amount — every ghost cert issued for that amount would have an invalid signature chain. Replaced with `pick_scheme()`, which resolves the naming scheme at the directory level, per amount. If a complete `notary_*` pair exists it's used; else if a complete `delegate_*` pair exists it's used with a deprecation warning; else it defaults to `notary_*` so the "not found" error points at the canonical filename. Six unit tests pin every resolution branch including the two partial-migration cases. Codex wrong-repository-URL fix (LOW #7) ======================================= `rust/gklib/Cargo.toml` listed `repository = "https://github.com/freenet/freenet-core"` which is wrong — the crate lives in `freenet/web`. Pre-existing but this PR publishes it, so fix in the same commit. Corrected to `https://github.com/freenet/web`. Testing reviewer HIGH #1 — unit tests for fallback resolvers ============================================================ Added `#[cfg(test)] mod resolve_tests` to `rust/cli/src/commands.rs` covering `resolve_notary_file` (4 tests: prefer canonical, fall back to legacy, not-found returns canonical, signing key variant) and `delegates::tests` in `rust/api/src/delegates.rs` (6 tests: prefer notary, fall back to legacy, refuse partial-notary pair, refuse partial-legacy pair, missing directory, filename format contract). Terminology: "wallet" → "vault" (product-name accuracy) ======================================================== Issue #24 referred to the Ghostkeys delegate as a "wallet delegate" but the actual product name in `freenet/ghostkeys/ui/` is "Ghostkey Vault" / "Identity Vault" (class names `vault-section`, `empty-vault`, page title "Ghostkey Vault"). Updated the gklib AGENTS.md terminology section and the CHANGELOG to call out three distinct concepts: - notary certificate (PKI intermediate, this crate) - Freenet `Delegate` (generic platform primitive, sandboxed WASM agent) - Ghostkey Vault (specific freenet/ghostkeys delegate, user-facing product name) None of my committed files ever said "wallet" in prose, but the AGENTS.md section was previously silent on the vault product name; now it specifically warns readers not to parrot issue #24's language. Miscellaneous cleanups from code-first + testing reviewers ========================================================== - `rust/integration_test/src/browser_test.rs:220` error-message string used "Delegate certificate validation failed" — now accepts both the legacy and the new error text so migrations work either way. - `rust/integration_test/src/environment.rs:29` set `GHOSTKEY_DELEGATE_KEY_DIR` alongside `NOTARY_DIR`, but nothing in the repo reads that env var — dead code from the pre-rename state, removed. - Updated `rust/gklib/src/delegate_certificate.rs` #[deprecated] `since` attributes from "0.1.5" to "0.2.0" to match the actual release. - Fixed inline "0.1.5" references in source doc comments to match the 0.2.0 target. Test results (all green locally) ================================ - `cargo test --manifest-path rust/gklib/Cargo.toml --all-features`: 22 unit + 6 legacy fixture compat tests pass - `cargo test --manifest-path rust/Cargo.toml --all-features`: 14 ghostkey-api tests pass (includes the 6 new pick_scheme tests), 3 gkwasm tests pass, integration_test builds - `cargo test --manifest-path rust/cli/Cargo.toml`: 4 new resolve_notary_file tests pass - `test_cli_commands.sh`: 17/17 green (both new + legacy spellings) Refs #24 [AI-assisted - Claude] * docs: sync remaining 0.1.5 references to 0.2.0 Follow-up to cc3b30f — caught a few more in-code doc comments referring to pre-0.1.5 that should have been pre-0.2.0. Pure documentation, no functional changes. Refs #24 [AI-assisted - Claude]
sanity
added a commit
that referenced
this pull request
Apr 30, 2026
Five-perspective review on PR #42 surfaced three issues with the initial fix: - Codex P2: discriminating only on `[ -t 0 ]` regresses `printf 'y' | sh install.sh` automation patterns. When stdin is piped but the script ran from a file, the user's answer is on stdin, not /dev/tty. - Skeptical #1: `read -r response` returning EOF (Ctrl-D, closed stdin) under `set -eu` aborts the script. - Skeptical #10: comment used an em-dash, which Ian's writing-style rule rejects. Redesigned the dispatch around `$0`: when sh runs a script file (file-execution), $0 is the script path; when sh reads its own source from stdin (`curl | sh`), $0 is the shell name. The case arm tests for known shell names and only redirects to /dev/tty in that branch. The file-execution arm reads from stdin as before, so piped automation still works. Both `read` calls now have `|| response=""` so EOF leaves the default-N case path intact rather than aborting. Also (lower priority feedback applied): - Skipped path now mentions FREENET_NO_SERVICE=1 as the documented way to bypass the prompt non-interactively (Skeptical #2/#3). - Removed the redundant "Non-interactive shell detected" info line (Skeptical #7); the existing `*)` case arm already prints "Skipping service installation" plus remediation guidance. - Added a NOTE block at the top of the file pointing future resync agents at the freenet-core sibling and explaining why the prompt block must not be flattened back to a plain `read` (Big-picture #5). Re-tested via the same pty harness across five scenarios: 1. `curl | sh` under pty: reads 'y' from /dev/tty, installs. 2. `sh install.sh` interactive: reads from stdin. 3. `printf 'y' | sh install.sh`: reads from piped stdin (regression test for Codex's concern). 4. `setsid ... </dev/null`: skips, prints FREENET_NO_SERVICE hint. 5. `sh install.sh </dev/null`: handles EOF without aborting. All five pass. [AI-assisted - Claude]
sanity
added a commit
that referenced
this pull request
Apr 30, 2026
* fix(install): read prompt from /dev/tty when stdin is piped When the install script is run via the documented `curl ... | sh` pattern, stdin is the pipe carrying script bytes, so `read -r response` cannot capture keystrokes and the service-install prompt silently ignores keyboard input (reported by Ivvor on Matrix). A previous fix (eff317ae, Dec 2025) addressed this with `< /dev/tty`, but PRs #36 / #37 resynced this file from freenet-core and dropped the override. Reapply the fix and harden it for environments without a controlling terminal. The new logic: - If stdin is a TTY, read normally (covers `sh install.sh`). - Else, probe /dev/tty by actually opening it (`{ true </dev/tty; }`) rather than relying on `[ -r /dev/tty ]`, which can return true even when the open later fails. If /dev/tty is openable, read from it (covers `curl | sh`). - Otherwise, skip the prompt rather than aborting under `set -eu` (covers truly non-interactive shells, e.g. CI or `setsid`). Verified via a pty harness: `curl | sh` simulation under `script(1)` reads "y" from /dev/tty and proceeds; `setsid ... </dev/null` skips the prompt and prints follow-up instructions. Note: freenet-core/scripts/install.sh has the identical bug; a matching fix will follow there to keep the resync in sync. [AI-assisted - Claude] * fix(install): address review feedback - $0 detection, EOF guard Five-perspective review on PR #42 surfaced three issues with the initial fix: - Codex P2: discriminating only on `[ -t 0 ]` regresses `printf 'y' | sh install.sh` automation patterns. When stdin is piped but the script ran from a file, the user's answer is on stdin, not /dev/tty. - Skeptical #1: `read -r response` returning EOF (Ctrl-D, closed stdin) under `set -eu` aborts the script. - Skeptical #10: comment used an em-dash, which Ian's writing-style rule rejects. Redesigned the dispatch around `$0`: when sh runs a script file (file-execution), $0 is the script path; when sh reads its own source from stdin (`curl | sh`), $0 is the shell name. The case arm tests for known shell names and only redirects to /dev/tty in that branch. The file-execution arm reads from stdin as before, so piped automation still works. Both `read` calls now have `|| response=""` so EOF leaves the default-N case path intact rather than aborting. Also (lower priority feedback applied): - Skipped path now mentions FREENET_NO_SERVICE=1 as the documented way to bypass the prompt non-interactively (Skeptical #2/#3). - Removed the redundant "Non-interactive shell detected" info line (Skeptical #7); the existing `*)` case arm already prints "Skipping service installation" plus remediation guidance. - Added a NOTE block at the top of the file pointing future resync agents at the freenet-core sibling and explaining why the prompt block must not be flattened back to a plain `read` (Big-picture #5). Re-tested via the same pty harness across five scenarios: 1. `curl | sh` under pty: reads 'y' from /dev/tty, installs. 2. `sh install.sh` interactive: reads from stdin. 3. `printf 'y' | sh install.sh`: reads from piped stdin (regression test for Codex's concern). 4. `setsid ... </dev/null`: skips, prints FREENET_NO_SERVICE hint. 5. `sh install.sh </dev/null`: handles EOF without aborting. All five pass. [AI-assisted - Claude]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had also set a easier way to navigate to the doc. I will merge all in hugo as soon as possible.