docs: re-cut README and relocate broker operations reference - #16
Merged
Conversation
A review fix round edited doc comments in apps/broker/src/main.rs to match the docs updated on this branch. The branch is docs-only by explicit constraint, so no file under a src/ directory may change here. The stale comment stands as a separate follow-up.
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
1 participant
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.
Intent
Docs-only re-cut of README.md so it reads as one designed document instead of accreted per-PR sections. Five ordered defects: (1) the ~48-line 'Privileged broker' section was out of proportion, so its filesystem-permission reference material (sticky bits, why /tmp and merely-traversable /run are refused, XDG_RUNTIME_DIR vs the single-instance lock, systemd RuntimeDirectoryMode 0755, recovery from wrong-owner /run/fpsmaxxing, flag/env precedence) was RELOCATED IN FULL to a new docs/BROKER_OPERATIONS.md and registered in the docs/README.md index table; the README keeps under 10 lines plus a link. Deleting that material was forbidden. (2) Tense was incoherent between the walkthrough and the status list; convention is now present tense for what is implemented, explicit future tense for what is not, with the alpha 'not on real hardware yet' caveat stated once prominently. (3) The same pipeline was drawn four times; the Mermaid diagram and the prose walkthrough stay, the redundant arrow text block and the ASCII architecture diagram were cut. (4) 'Repository status' was a 13-bullet merge-history changelog and is now organised by capability. (5) The first FAQ answer was a five-sentence wall and now leads with the direct answer then the caveat. HARD CONSTRAINT, captain-set and machine-verified by 'git diff --name-only origin/main...HEAD': this branch touches documentation only. No file under any src/ directory and no file under crates/ may change, including doc comments. The previous run's review round edited apps/broker/src/main.rs doc comments and commit 4ae6b8d reverts exactly that; do not re-apply it. The stale JOURNAL_ENV doc comment in apps/broker/src/main.rs is a known, deliberate out-of-scope follow-up, not a defect of this branch. Also protected by direct captain order: the '## The closed loop' section and its Mermaid diagram must stay near the top, byte-identical to origin/main. Repo Markdown convention is one full sentence per line and plain dashes only; README must contain zero em dashes. Everything else on this branch already cleared a full review, test, document, lint, push, PR, and CI cycle as PR #15.
What Changed
README.mdas a single document: the closed-loop walkthrough and status list now use present tense only for what runs on the mock path and explicit future tense for what does not, the redundant arrow-text pipeline and ASCII architecture diagram are cut (the Mermaid diagram is byte-identical tomain), "Repository status" is regrouped by capability instead of merge history, and the first FAQ answer leads with the direct answer before the alpha caveat. CorrectedFPSMAXXING_JOURNAL_PATHownership to gateway, CLI, and watchdog, and narrowed the broker's platform refusal to Windows.docs/BROKER_OPERATIONS.md(socket/journal/lock paths, private-directory ownership and mode rules, why/tmpand/runare refused,XDG_RUNTIME_DIRversus the single-instance lock, systemdRuntimeDirectorysettings, flag/env precedence). The README keeps a short broker section plus a link; the page is registered in thedocs/README.mdindex and linked fromdocs/ARCHITECTURE.md.coreandcore.*to.gitignoreafter a 19 MB crash dump was accidentally committed and removed earlier on this branch.Documentation-only apart from the
.gitignoreline; no file undercrates/or anysrc/directory changed, and commit 4ae6b8d reverts an earlier run's broker doc-comment edit to keep that constraint. The staleJOURNAL_ENVdoc comment inapps/broker/src/main.rs:100is a known out-of-scope follow-up.Risk Assessment
✅ Low: Docs-only change with the accidental core dump purged from history; every relocated permission, ownership, and lifecycle claim now checks out against apps/broker, apps/watchdog, and apps/experiment-runner, leaving only two cosmetic redundancy items.
Testing
Rendered the README before and after to GitHub-styled HTML and captured screenshots showing all five defects fixed: the 49-line broker section is down to 11 with its reference material relocated intact to a new docs/BROKER_OPERATIONS.md, the alpha caveat now leads once up front with present/future tense split consistently, only one Mermaid pipeline drawing remains, "Repository status" is grouped by capability, and the first FAQ answer opens with "Not yet on real hardware." before the caveat. Drove the new README -> broker-ops -> back-to-anchor link path in a real browser with no console errors, and machine-checked the hard constraints: docs-only diff, apps/broker/src/main.rs byte-identical to origin/main, the closed-loop Mermaid block byte-identical by md5, zero em dashes. Then built the workspace with local zig shims and ran the README's own documented commands, which confirms its present-tense claims: doctor flips from journal missing to ready, the MCP snippet returns a verified and rolled-back lifecycle, the experiment runner promotes and replays consistently, the watchdog passes both exit 0, and the broker enforces every ownership, sticky-bit, traversal, single-instance and env-var rule the relocated page documents.cargo test --workspaceis fully green. No product failures; the only leftovers are an untracked pre-existing core dump and seven multi-sentence README lines inherited unchanged from origin/main./tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/00-readme-recut-before-after-by-defect.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/01-readme-BEFORE-full.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/02-readme-AFTER-full.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/03-BROKER_OPERATIONS-new-page.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/04-docs-index-operations-row.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/05-nav-step1-README-privileged-broker.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/06-nav-step2-BROKER_OPERATIONS-top.png)/tmp/no-mistakes-evidence/01KYNK9V5JXXXHP3HPSBH74FJT/07-nav-step3-backlink-lands-on-anchor.png)Evidence: CLI transcript - the README's documented commands run verbatim
$ cargo run -p fpsmaxxing-cli -- doctor FPSMaxxing diagnostics contracts: ready provider SDK: ready gateway: MCP mock path available journal: missing (no experiments journaled yet) (/tmp/tmp.xPgTc7whOC/journal.sqlite) hardware writes: disabled status: read-only alpha mock path ready $ printf ... | cargo run -p fpsmaxxing-gateway # README MCP snippet {"id":1,"jsonrpc":"2.0","result":{"tools":[ {"name":"fpsmaxxing.capabilities", "description":"Discover typed, policy-approved capabilities"}, {"name":"fpsmaxxing.run_mock_lifecycle", "description":"Run snapshot, preview, apply, verify, and rollback for mock.value"}]}} {"id":2,"jsonrpc":"2.0","result":{"content":[{"type":"text", "text":"{"provider_id":"mock","preview":"set mock.value from 0 to 42","verified":true,"rolled_back":true}"}]}} $ cargo run -p fpsmaxxing-cli -- doctor # after the lifecycle ran journal: ready (/tmp/tmp.xPgTc7whOC/journal.sqlite) $ cargo run -p fpsmaxxing-experiment-runner trial 1 -> Promote (Promoted); fps_improvement = 30.0 lifecycle: provider mock, verified = true, rolled_back = true replay 1 -> recomputed Promote; consistent with journal = true, policy legal = true exit=0 $ cargo run -p fpsmaxxing-watchdog -- --once exit=0 $ cargo run -p fpsmaxxing-watchdog -- --recover-all exit=0Evidence: Every claim relocated into docs/BROKER_OPERATIONS.md, checked against the running broker binary
=== 2. "RuntimeDirectoryMode defaults to 0755 ... a unit that omits the mode is refused on every start" === $ XDG_RUNTIME_DIR=/tmp/tmp.jGy8fIS7KW # private dir pre-created 0755, exactly what systemd does by default exit=1 :: fpsmaxxing-broker: /tmp/tmp.jGy8fIS7KW/fpsmaxxing must be a directory owned by uid 10001 with mode 700 $ chmod 0700 $XDG_RUNTIME_DIR/fpsmaxxing && rerun exit=124 (still serving when the 3s timeout fired) :: listening on .../fpsmaxxing/broker.sock (same-uid ACL, uid 10001) === 3. "a shared root like /tmp is refused" === exit=1 :: fpsmaxxing-broker: /tmp must be a directory owned by uid 10001 or root that no other user can reach it === 4. "a merely traversable directory like /run ... is refused too" === exit=1 :: fpsmaxxing-broker: /run must be a directory owned by uid 10001 or root that no other user can reach it === 5. "Only one broker may run per user ... a second broker exits non-zero" === second broker exit=1 :: broker.lock is held by a running broker; only one may own this machine's knobs === 6. "the lock is not derived from --socket or --journal" === second broker, different --socket and --journal: exit=1 :: broker.lock is held by a running broker === 7. "the privileged broker deliberately does not read FPSMAXXING_JOURNAL_PATH" === journal the broker actually wrote: /tmp/tmp.jGy8fIS7KW/fpsmaxxing/journal.sqlite path from FPSMAXXING_JOURNAL_PATH exists? no - broker ignored it, as documentedEvidence: Hard-constraints check (docs-only, Mermaid byte-identity, em dashes, sentence-per-line, relocation size, link resolution)
--- C1: documentation only --- README.md / docs/ARCHITECTURE.md / docs/BROKER_OPERATIONS.md / docs/README.md verdict: PASS - no file under any src/ or crates/ directory changed --- C1b: the 4ae6b8d revert was not re-applied --- apps/broker/src/main.rs vs origin/main: PASS - byte-identical stale JOURNAL_ENV doc comment: yes, untouched as required --- C2: closed-loop Mermaid byte-identical --- e311e9ff1b60e526b9166c96fa855cf1 origin/main e311e9ff1b60e526b9166c96fa855cf1 HEAD first H2 is 15:## The closed loop --- C3: em dashes --- README.md: 0 docs/BROKER_OPERATIONS.md: 0 --- C5: relocation preserved, not deleted --- origin/main README broker section: 49 lines HEAD README broker section: 11 lines (3 prose, one of which is the link) docs/BROKER_OPERATIONS.md (new): 66 lines --- C6: pipeline drawn once --- mermaid blocks in README: 1 ASCII "## Architecture" block: removed "observe -> propose -> ..." arrow block: removedPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
docs/BROKER_OPERATIONS.md:9- The "gateway does not reach the broker yet" status now lives in four places: README.md:10 (IMPORTANT block), README.md:37 (walkthrough), README.md:126 (broker section), and docs/BROKER_OPERATIONS.md:9. The point of the re-cut is one coherent document; four copies of the same volatile status line will drift the moment the gateway is wired to the broker, and BROKER_OPERATIONS.md:9 also duplicates README.md:126 nearly verbatim including theapps/broker/tests/integration.rspath. Suggest stating it once (walkthrough) and having the broker section plus BROKER_OPERATIONS defer to it.docs/README.md:18- README.md:127 and docs/README.md:18 both advertise the new page as covering "systemd units", but docs/BROKER_OPERATIONS.md contains exactly one systemd sentence (line 58: RuntimeDirectory + RuntimeDirectoryMode) and no unit file or unit-level guidance. Either reword both link descriptions to "systemd RuntimeDirectory requirements" or add a minimal example unit.README.md:93- README.md:93 says the broker is "on the Linux-safe path only" while README.md:125 (correctly, matching thecfg(not(unix))gate at apps/broker/src/main.rs:40) says it refuses only on Windows. The base README had no platform qualifier on this bullet, so the new wording introduces the tension; a reader can conclude the binary refuses on macOS/BSD when it does not. "Linux-safe" is existing repo vocabulary (docs/ARCHITECTURE.md:6), so this is only about the two statements sitting in the same document.README.md:38- "today that in-process control plane links its provider directly" grammatically refers to the gateway's in-process plane from line 37, which leaves the impression the broker already reaches a sidecar when driven by BrokerClient. It does not: apps/broker/src/main.rs:215-217 boxes MockProvider straight into its own ControlPlane. Consider "today both the gateway's and the broker's control planes link their provider directly."🔧 Fix: defer broker doc status to README, fix systemd wording
2 infos still open:
README.md:87- README:87 and README:97 both hang a load-bearing status claim on "the walkthrough" ("except what the walkthrough marks as future", "Everything the walkthrough above states in the future tense"), but no section in the README is ever labelled a walkthrough. The intended referent is the prose at README:34-43 under "## The closed loop". Since line 87 is the one sentence that bounds how much of the closed loop actually works, a reader who cannot resolve the term cannot check the claim. Naming it explicitly ("except what the closed-loop walkthrough above marks as future") closes it without touching the protected diagram or section.docs/BROKER_OPERATIONS.md:33- The new page's central term is introduced after it is used.<private dir>appears in the defaults column at lines 33-34, and "the default private directory already is one" at line 39, but "## The private directory" does not define it ($XDG_RUNTIME_DIR/fpsmaxxing, or/run/fpsmaxxingunder root) until line 49. In the old README section the private-directory prose preceded the table, so the order was correct; the relocation reversed it. Moving "## The private directory" ahead of "## Socket and journal paths" restores the reading order with no wording change.🔧 Fix: name walkthrough referent, reorder broker ops sections
3 issues (1 warning, 2 infos) still open:
docs/BROKER_OPERATIONS.md:51- "held to the same bar as the default" is new on this branch (the base README said only "held to the same bar") and the added comparison is wrong on the ownership axis. In apps/broker/src/main.rs, vet_directory accepts a parent whose owner ismetadata.uid() == broker_uid || metadata.uid() == ROOT_UID, while private_directory_in accepts the private directory only whenmetadata.uid() == broker_uid. Line 36 of this same page states that distinction explicitly ("root ownership satisfies an explicit --socket or --journal parent, but a broker accepts its private directory only when it owns that itself"), so the page now contradicts itself 15 lines apart on a security check. Mode and ancestor bars do match; only ownership differs. Dropping "as the default" restores the accurate base wording.docs/BROKER_OPERATIONS.md:33- After the round-2 reorder, lines 31-33 are three consecutive sentences carrying the same fact. Line 31 defines the private directory as$XDG_RUNTIME_DIR/fpsmaxxingor/run/fpsmaxxing; line 32 says the broker creates it "whether or not an override moved the socket and the journal out of it, because the single-instance lock lives there"; line 33 then repeats both ("still establishes that directory even when both paths are given", and re-spells the same two paths). In the base README these two sentences sat on opposite sides of the settings table, so the repetition was not visible; the relocation made them adjacent. Line 33 adds only "on every start" and can fold into line 32.README.md:12- The new caveat asserts a universal convention - "Every claim below about what the system does is written in the present tense for what runs today on that mock path, and in the future tense for what does not exist yet" - and carves out only the design principles and the planned integrations. The FAQ is not carved out and violates it: README.md:151 reads "Planned GPU operations use supported vendor APIs, query device limits first, require rollback, and run under an independent thermal watchdog" and "Persistent or high-risk operations require explicit approval", both present tense for capabilities that do not exist (no GPU provider, no approval flow in the workspace). Since defect 2 of this re-cut was tense coherence, either extend the carve-out to name the FAQ or shift those two sentences to future tense.🔧 Fix: fix broker directory ownership claim, cut duplicate sentence
2 issues (1 error, 1 warning) still open:
core:1- Commit 2838ed3 committed a 19,824,640-byte ELF core dump (core) at the repository root alongside its 3-line docs edit. Confirmed: ELF e_type == 4 (ET_CORE), and embedded strings identify it as a crash of/home/fleet/.local/libexec/sentrux check .(the Stop-hook quality gate), not project code. Three problems: (1) it breaks the branch's docs-only constraint -git diff --name-only 9e76b27..HEADlistscorenext to the four Markdown files; (2) once merged the 19 MB blob is in history permanently and cannot be removed without a rewrite, and every future clone pays for it; (3) a core dump is a snapshot of process memory, so it can contain repository file contents, environment variables, and any token sentrux had loaded - publishing it to a shared remote is an unintended disclosure. Fix: drop the blob from the commit rather than deleting it in a follow-up commit (git rm --cached core && rm core, then amend 2838ed3 so the blob never enters history), and addcorepluscore.*to .gitignore, which currently has no coredump pattern.docs/BROKER_OPERATIONS.md:32- Line 32 says the broker "refuses to start unless it and every directory above it are owned by the broker or root and are not writable by anyone else", where "it" is the private directory. That is wrong for the private directory on both axes, and the page contradicts itself on the same security check within twenty lines. Ownership:private_directory_in(apps/broker/src/main.rs:508-511) accepts the directory only whenmetadata.uid() == broker_uid, so a root-owned/run/fpsmaxxingis refused for a non-root broker - exactly what line 35 and the round-3 rewrite at line 51 both state. Mode: the same check requiresmetadata.mode() & 0o777 == PRIVATE_DIR_MODE(exact 0700, so no other-user read or traverse either), which is stricter than "not writable by anyone else"; only the ancestors get theBar::NoForeignWritebar that sentence describes. The wording is inherited from the base README, but there the correcting sentence sat on the far side of the settings table; the round-2 reorder put the two three lines apart. Narrowing line 32's clause to the ancestors and letting line 35 keep the private-directory rule resolves it without touching the code.🔧 Fix: drop committed core dump, fix private-directory ownership rule
2 infos still open:
README.md:97- README:87 ("Every stage of the closed loop above has a working implementation on the mock path, except what the closed-loop walkthrough above marks as future") and README:97 ("Everything the closed-loop walkthrough above states in the future tense is still ahead of us") are the same claim stated twice, inverted, ten lines apart with only the capability list between them. Line 98 ("So are real hardware providers and live frame-time measurement") is the only new information in that closing pair, and it depends on line 97 for its "So are". Since the point of the re-cut was one designed document rather than accreted statements, folding 97 into 98 ("Real hardware providers and live frame-time measurement are also still ahead of us") keeps the new fact and drops the restatement..gitignore:1- The 19 MBcoredump was correctly removed from branch history, but .gitignore still carries nocore/core.*pattern, so the next sentrux Stop-hook crash drops another dump at the repo root where agit add -Apicks it up again. Adding the pattern would put a non-Markdown file in the diff and break this branch's machine-verified docs-only constraint, so it is a deliberate tradeoff rather than an oversight; worth a one-line follow-up commit on main instead.core- An untracked 19 MBcoredump sits at the repo root. It predates this test run (it was already in git status at session start) and it is not in the target commit, but it is also not covered by .gitignore, so a futuregit add -Awould re-commit it - which is exactly what commit 93f43c6 had to undo. Removing it is a deletion, so I left it in place rather than deciding for you.README.md:136- Seven README lines still carry more than one sentence, against the repo's one-sentence-per-line convention: L136 (Documentation), L151/L155/L159/L163 (FAQ answers 2-5), L167 (Contributing), L171 (Security). All seven are byte-identical to origin/main and untouched by this branch, so they are pre-existing and out of its scope. Noting them only so the convention gap is not mistaken for something this branch introduced.git diff --name-only 9e76b27..HEAD- confirmed only README.md, docs/ARCHITECTURE.md, docs/BROKER_OPERATIONS.md, docs/README.md changed; grep for(^|/)(src|crates)/returns nothinggit diff --quiet 9e76b27..HEAD -- apps/broker/src/main.rs- byte-identical, so the 4ae6b8d revert was not re-applied;grep JOURNAL_ENV apps/broker/src/main.rsconfirms the known stale doc comment is untouchedawk '/^```mermaid/,/^```$/' README.md | md5sumcompared against the same extraction fromgit show 9e76b27:README.md- both e311e9ff1b60e526b9166c96fa855cf1grep -c '—' README.md docs/BROKER_OPERATIONS.md- zero em dashes in bothPython link/anchor walker over README.md, docs/README.md, docs/BROKER_OPERATIONS.md, docs/ARCHITECTURE.md - every relative file target and every#fragmentresolves to a real headingRendered README.md at 9e76b27 and at HEAD, plus docs/BROKER_OPERATIONS.md and docs/README.md, to GitHub-styled HTML (marked + github-markdown-css + mermaid 11) and captured full-page screenshots with headless ChromiumComposed and screenshotted a five-row before/after comparison page, one row per stated defectBrowser-driven navigation: load README, scroll to#privileged-broker, click "Broker operations and deployment", asserth1is "Broker operations and deployment", click the backlink, assert URL isREADME.html#privileged-brokerand the anchor sits at offset 0px; zero console errorscargo build -p fpsmaxxing-gateway -p fpsmaxxing-cli -p fpsmaxxing-broker -p fpsmaxxing-watchdog -p fpsmaxxing-experiment-runnerwith local zig CC/AR shims to work around the libsqlite3-sys build-script failurecargo run -p fpsmaxxing-cli -- doctorbefore and after the lifecycle - journal reportedmissingthenreadyThe README's MCP snippet verbatim:printf '%s\n' '{...tools/list...}' '{...tools/call fpsmaxxing.run_mock_lifecycle...}' | cargo run -p fpsmaxxing-gatewaycargo run -p fpsmaxxing-experiment-runner- promoted trial with consistent replay, exit 0cargo run -p fpsmaxxing-watchdog -- --onceand-- --recover-all- both exit 0fpsmaxxing-broker --help, plus refusal cases for a 0755 private directory,--socket /tmp/...,--socket /run/..., a second concurrent broker, a second broker with different--socket/--journal, andFPSMAXXING_JOURNAL_PATHbeing ignoredcargo test --workspace- 170 tests across 20 targets, all passing, includingapps/broker/tests/integration.rswhich the README now namesapps/broker/src/main.rs:100- apps/broker/src/main.rs still documents FPSMAXXING_JOURNAL_PATH as belonging to "the unprivileged gateway and CLI" (JOURNAL_ENV doc comment at :100-104, resolve_paths_from doc comment at :262, test constant doc at :695), but apps/watchdog/src/main.rs:111 reads that variable too. README.md:115 and docs/ARCHITECTURE.md:56 were both corrected to "gateway, CLI, and watchdog" on this branch; the code doc comments were not. Left unfixed deliberately: this branch is under a machine-verified docs-only scope constraint (git diff --name-only must touch no file under crates/ or any src/), and commit 4ae6b8d reverts exactly this edit from a prior run. Needs a human call on whether to land it as a separate non-docs PR.🔧 **Lint** - 1 issue found → auto-fixed ✅
core- A 19,804,160-byte aarch64 ELF core dump is committed at the repository root, added by target commit 80942ec (Bin 0 -> 19804160 bytes) and currently tracked by git. It is unrelated build/crash debris on a branch whose stated constraint is documentation-only, it bloats every clone, and.gitignorehas nocorepattern so it can be re-added. Commit 93f43c6 two commits earlier is titled "drop committed core dump", so the removal intent is already established. Fix:git rm --cached core && rm core, then appendcoreandcore.*to.gitignore. Not auto-applied: deleting a tracked file is destructive, and adding a.gitignoreentry is a non-documentation change on a docs-only branch, so both need author sign-off.🔧 Fix: drop core dump from history, ignore future dumps
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.