Skip to content

Document and operationalize production-readiness remediation program - #75

Merged
plx merged 5 commits into
mainfrom
plx/dhaka
Jul 25, 2026
Merged

Document and operationalize production-readiness remediation program#75
plx merged 5 commits into
mainfrom
plx/dhaka

Conversation

@plx

@plx plx commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Situation

This PR records the broad pre-release due-diligence review of agentic-navigation-guide, establishes the issue-driven path from a lightly used experimental tool to a defensible 0.2.0 release, and adds a fail-closed helper for choosing remediation work in priority and dependency order.

The audit's verdict is deliberately conservative: the repository is a credible pre-1.0 Rust foundation and does not need a rewrite, but audited revision bf40806993ce0e0f2de931cabd105e3a0e063789 is not ready for the next formal release or for use as a load-bearing workplace verification gate.

The static documentation site and landing page were excluded as requested. Cargo package-boundary behavior remains in scope because the current crate package includes the site and other unrelated repository content.

Scope of the findings

The release-blocking correctness and boundary defects include paths that can:

  • attach an indented child to a different directory than the guide expresses;
  • make dump/init emit guides that do not round-trip through the parser;
  • let a malformed opening marker activate ignore=true and bypass validation;
  • report recursive verification success after finding zero guides;
  • read a guide symlink outside the expected discovery boundary; and
  • write init output through a dangling symlink, with a separate check/write race.

The report also identifies inconsistent ignore/exclusion/path semantics, unfinished or incorrect public Rust API behavior, cross-platform and resource-scaling gaps, a non-hermetic test, mutable or overprivileged workflows, an unenforced MSRV, an overbroad Cargo package, missing release automation/provenance, the already-published 0.1.4 version and SemVer break, contradictory immutable 0.1.4 licensing metadata, incomplete user/security/contributor documentation, and maintainer-recovery risk.

What this PR adds

This PR changes no product runtime behavior and claims no audited defect is fixed. It adds planning records and maintainer orchestration for the follow-up PR sequence.

Ticket program

The remediation program is tracked in milestone v0.2.0 — production-readiness hardening and top-level epic #26.

It contains 49 open program issues arranged as a complete native sub-issue tree:

The component epics cover grammar/parser; dump/init/CLI; verifier/discovery/filesystem safety; public API/SemVer; cross-platform testing/performance; release/supply chain; and docs/governance/distribution. Contract decisions #34#36 intentionally lead implementation. Independent reassessment #72 is blocked by all component epics, publication #73 is blocked by that audit PASS and the release controls, and Homebrew #74 follows the live audited release. Because #74 remains a native child of #26, the overall program epic closes only after post-publication verification; it does not delay the #73 publication gate.

Every ticket has current behavior/impact, required implementation direction, red-before-fix regression expectations where applicable, validation steps, acceptance criteria, dependencies, and non-goals. Draft PR #21 is cross-linked as prior art where it overlaps exact markers, get_full_path, README cleanup, and historical specification handling; it is not silently treated as completed remediation.

Labels and dependencies

The semantic taxonomy combines:

  • exactly one priority (P0P3);
  • component: …, domain: …, and where relevant risk: … labels;
  • production-readiness, release-blocker, needs-decision, testing, hardening, or release workflow labels as applicable;
  • target: v0.2.0 versus target: post-v0.2; and
  • automatic-selection membership plus exactly one leaf/gate role.

All 49 tickets carry production-readiness and workflow:production-readiness; 39 are workflow:production-readiness-leaf, and #26#33, #72, and #73 are the 10 workflow:production-readiness-gate issues. The redundant 49-issue cohorts are required to match exactly, preventing a dropped or stray label from silently changing the queue.

GitHub-native sub-issue relationships encode ownership, and 84 native blocked by relationships encode cross-ticket prerequisites. The selector treats both blockers and native children as hard requirements for gates, which prevents an organizing epic from becoming actionable before all of its children have actually closed. Forty-eight issues belong to milestone #2; #74 is intentionally outside it.

Automatic burndown selection

After this PR merges, the normal operator loop starts with:

just get-next-production-readiness-issue

--json provides stable machine-readable state, and repeatable --exclude N skips an otherwise-ready issue for one invocation without mutating GitHub.

The selector reads live issue state, exact labels, native blockers/sub-issues, the current default branch, and GitHub's authoritative closingIssuesReferences. It does not schedule from body prose, checklists, milestones, or project-board position.

Ordering is P0 → P1 → P2 → P3, then leaf before gate, then issue number. An open default-branch PR (including a draft) covers exactly one issue only when GitHub recognizes a closing keyword. Covered leaf chains can sequence downstream leaf work, but every gate blocker and sub-issue must be actually closed. Closing PR coverage is work sequencing, never merge authorization.

The implementation fails closed on cohort/taxonomy mismatches, label-role collisions, graph truncation, external open blockers, ambiguous PR coverage, pagination changes, default-branch changes, and unstable snapshots. Before returning a dependent issue it now revalidates the exact transitive covered-leaf PR proof that made the issue ready, closing a narrow race inherited from the reference implementation.

The current production run selects #34 from 49 open issues, with zero covered and 11 ready.

Live integration validation and cleanup

A namespaced alternate-label smoke run exercised four fake issues and four draft, never-merged PRs without putting fixtures into the production cohort:

The full 11-transition assertion table is retained in the work-selection guide. Cleanup closed PRs #80#83 without merge, closed issues #76#79 as completed, deleted all four fixture branches and temporary labels, and removed production priority labels from the fixtures. The only retained artifacts are the closed issue/PR evidence and consumed numbers.

Post-cleanup verification confirmed:

Final reassessment gate

The playbook does more than rerun the current suite. It requires a fresh-clone, exact-commit evidence ledger; red-before-fix reproduction of original defects; parser/generator/verifier/CLI/API adversarial checks; Linux/macOS/Windows and MSRV validation; property/fuzz/mutation/coverage/performance work; symlink, containment, concurrency, and resource-boundary checks; package/install/artifact smoke tests; dependency/license/SemVer review; workflow/repository/recovery/security-report inspection; release dry run; and an explicit PASS/CONDITIONAL/FAIL report.

A false-success, containment escape, unsafe publish path, unsupported claimed platform, unresolved P0/P1 defect, or untested release artifact forces a release FAIL. Substantive defects discovered during reassessment must receive separate issues and fixes, followed by rerun evidence; they must not be hidden inside the audit PR.

Validation

  • 61 selector tests pass on Python 3.10, 3.12, and 3.14.
  • just --fmt --check
  • ruff check scripts/get_next_production_readiness_issue.py tests/test_get_next_production_readiness_issue.py
  • ruff format --check scripts/get_next_production_readiness_issue.py tests/test_get_next_production_readiness_issue.py
  • python3 -m py_compile scripts/get_next_production_readiness_issue.py
  • cargo fmt --all -- --check
  • cargo test --locked — 127 tests pass; 2 doctests intentionally ignored.
  • cargo clippy --locked --all-targets -- -D warnings
  • cargo run --locked -- check --guide AGENTIC_NAVIGATION_GUIDE.md
  • cargo run --locked -- verify --guide AGENTIC_NAVIGATION_GUIDE.md --root .
  • markdownlint audits/*.md
  • lychee --no-progress audits/*.md — 70 links checked, 0 errors.
  • git diff --check
  • live production selector: Define the normative v0.2 guide grammar and filesystem representation contract #34, 49 open, 0 covered, 11 ready.

The original audit also records its baseline Rust and repository checks with exact results. Review of this PR should focus on whether the evidence, ticket decomposition, dependency order, selector policy, and final gate are complete and defensible. Merging it preserves and operationalizes the plan; it does not authorize publication.

@plx plx added discovered-by-codex Issues discovered via review by codex. documentation Improvements or additions to documentation production-readiness Part of the audited path from prototype to production-ready use labels Jul 25, 2026 — with ChatGPT Codex Connector
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This PR is documentation-only (two new files under audits/, plus a 3-line AGENTIC_NAVIGATION_GUIDE.md update) and touches no runtime code, so I focused the review on accuracy of the audit's technical claims and the quality of the documents themselves rather than code style/tests.

Verification of the headline claims

I spot-checked several of the "release blocker" findings against the actual source at the audited revision, and they all held up:

  • Finding 1 (hierarchy reparenting, src/parser.rs): confirmed. The parent-search loop (src/parser.rs:637-655) walks backward looking for any directory at current_level - 1 and only stops early if it passes a shallower level — it does not stop at a same-level non-directory sibling. The a/, b, - c repro in the report does exactly what's claimed: c silently attaches to a instead of being rejected because its lexical parent b is a file.
  • Finding 3 (malformed opening marker enabling ignore=true): confirmed. extract_guide_block only checks trimmed.starts_with("<agentic-navigation-guide") (src/parser.rs:55), and parse_ignore_attribute strips that same prefix with no required delimiter before it (src/parser.rs:106-117), so <agentic-navigation-guideignore=true> really does parse as an opening tag with ignore=true.
  • Finding 4 (recursive verify succeeds on zero guides): confirmed at src/cli/verify.rs:249-258 — the empty-guides branch prints a warning (or nothing, under --quiet) and returns Ok(()).
  • Finding 5 (guide symlink read boundary): confirmed — WalkDir::new(root).follow_links(false) (src/recursive.rs:54) is paired with path.is_file() (src/recursive.rs:61), which does follow the final symlink component, so a guide-named symlink pointing outside the walked root is still picked up and read.
  • Finding 6 (init dangling-symlink / TOCTOU): confirmed — src/cli/init.rs checks self.output.exists() (false for a dangling symlink) and later writes via fs::write, which follows symlinks, as two separate operations.

Given that 5-for-5 of the sampled critical findings check out precisely against current source, I have high confidence in the rest of the report's technical accuracy; it reads as genuinely investigated rather than templated.

I also checked that the cross-referenced tracking issues (#26, #72) resolve and match the titles quoted in the doc, so those aren't dangling references.

Minor nits (non-blocking)

  • Line-anchor drift: several #L<N> GitHub links are off by 1-2 lines from the actual construct they describe (e.g. src/types.rs#L116 for get_full_path, which is actually on line 117; src/parser.rs#L259 for the trim_end_matches('/') call, actually line 261; src/recursive.rs#L124 for the read_to_string call, actually line 123). Not misleading enough to matter for a reader skimming the surrounding context, but worth a quick pass if there's ever a mechanical way to regenerate these anchors before the next audit revision, since drift will only grow as remediation PRs land.
  • "an historical record" (line 48 of the audit) — stylistic only, a historical is more consistent with modern usage; not worth a re-review cycle on its own.

Process observations

  • Per CLAUDE.md's doc-alignment rule, this PR correctly makes no README.md changes since it changes no user-facing behavior — the audit's own scope note (site/ excluded, package-boundary behavior in scope) is consistent with that.
  • The AGENTIC_NAVIGATION_GUIDE.md addition accurately reflects the two new files actually added (audits/2026-07-25-production-readiness-audit.md, audits/production-readiness-reassessment-playbook.md), and cargo run -- check against it would pass.
  • The playbook (1296 lines) is thorough to the point of being close to a general-purpose Rust-CLI production-readiness template; that's arguably a feature (reusable rigor) but worth confirming the team is prepared to actually execute something this heavy for the reassessment gate (Execute an independent post-remediation production-readiness audit #72) rather than trimming it under time pressure later.

Overall: this is a well-executed, verifiably accurate audit with a credible, appropriately fail-closed verdict and a concrete, traceable remediation program. No changes requested from me.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This PR is documentation-only (adds two audit documents + a 3-line navigation-guide update), so the usual "bugs/perf/security" review criteria mostly reduce to: are the claims in the audit accurate, and is the document itself well-formed? I spot-checked a representative sample of the audit's specific, falsifiable claims against the actual source at the audited revision (bf40806, which matches current main for src/, README.md, Cargo.toml, and the workflows):

  • Finding 1 (hierarchy reparenting) — confirmed. build_hierarchy in src/parser.rs:637-654 searches backward and only stops early when it finds a directory at the target level or overshoots past it; a non-directory item at the target level is silently skipped rather than rejected, exactly as described.
  • Finding 2 (dump/parse non-round-trip) — confirmed. format_tree in src/dumper.rs:217-230 writes child.name verbatim with no escaping of parser metacharacters (#, [, ], ..., etc.).
  • Finding 3 (malformed opening marker enables ignore) — confirmed. trimmed.starts_with("<agentic-navigation-guide") (src/parser.rs:55) plus a prefix-strip in parse_ignore_attribute (src/parser.rs:106-113) means <agentic-navigation-guideignore=true> really does parse as an opening tag with ignore=true.
  • Finding 5 (guide symlink escapes discovery boundary) — confirmed. WalkDir::new(root).follow_links(false) (src/recursive.rs:54) is paired with path.is_file() (src/recursive.rs:61), which itself follows the final symlink component, so a symlinked guide name is read via fs::read_to_string (src/recursive.rs:124) regardless of where it points.
  • Finding 6 (init dangling-symlink write) — confirmed. src/cli/init.rs:45 uses self.output.exists(), which returns false for a dangling symlink, ahead of a later fs::write.
  • README contradictions (quickstart missing --output, "skip all syntax and semantic validation" promise) — both confirmed against README.md:178 and README.md:205.
  • CI workflow claims — confirmed: ci.yml pins actions/checkout to a commit SHA with a version-tag comment, while verify-guide.yml:14-15 uses mutable @v4/@v1 tags, matching the "inconsistent pinning" finding.

Every claim I checked (7 of the ~25 cited file:line references, including the highest-severity release blockers and the security-sensitive symlink findings) matched the code exactly, including subtleties like why the backward-search loop fails to stop at a file boundary. That's a strong signal the rest of the document is similarly grounded rather than speculative — nice work.

Minor notes (non-blocking)

  • A couple of citations are off by one line relative to the actual statement they're illustrating (e.g. src/cli/init.rs#L44 points at the comment above the self.output.exists() check, which is actually on line 45). Harmless for a reader, but worth a pass before these links get long-lived value as permanent historical records.
  • The PR is ~2000 lines of prose across two files, which is a lot of surface for a single human reviewer to fact-check exhaustively. Given the doc explicitly says it "changes no runtime code and claims no defect is fixed," and each release-blocker finding includes a concrete required-direction section that will presumably map to red/green regression tests in the linked tickets, that seems like an acceptable tradeoff rather than a defect in the PR itself.
  • Per CLAUDE.md's documentation-alignment rule, since this PR changes no user-facing behavior, no README.md update is required — that's correctly reflected here.
  • The AGENTIC_NAVIGATION_GUIDE.md update is accurate and consistent with the existing style (inline # comments, trailing .../ catch-all preserved).

Verdict

No factual or structural issues found in the portions I verified. This looks like a solid, well-evidenced audit that appropriately doesn't overreach into fixing anything itself, and the remediation/reassessment split (playbook explicitly requiring red-before-fix evidence, hard gates, and a non-waivable severity rubric for correctness/security findings) is a sound structure for the follow-up work. Approving from a documentation-quality and factual-accuracy standpoint.

@plx plx changed the title Document production-readiness audit and remediation program Document and operationalize production-readiness remediation program Jul 25, 2026
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This PR is documentation/process-only, as advertised — the diff touches .github/workflows/ci.yml, .gitignore, AGENTIC_NAVIGATION_GUIDE.md, CLAUDE.md, justfile, the two new audits/*.md records, and a new Python selector tool (scripts/get_next_production_readiness_issue.py + its tests). No src/ code changed, matching the PR description's claim of "no runtime code changes."

scripts/get_next_production_readiness_issue.py

  • Clean separation between pure logic (select_next, _sequenced_covered_ids, _coverage_proof, normalize_work_issues) and I/O (GitHubClient), which makes the 40+ unit tests in tests/test_get_next_production_readiness_issue.py meaningful rather than mocked end-to-end.
  • Subprocess calls to gh use argument lists (never shell=True / string interpolation), so there's no command-injection surface even though issue numbers and labels flow into -f/-F GraphQL variables.
  • The "stabilize across two consecutive snapshots" + freshness-guard design in get_selection is intricate (4-snapshot cap, retry counting, oscillation detection) but each branch has a corresponding test (StableSnapshotTests), including the trickier cases like a covered leaf whose upstream coverage disappears between snapshots and same-number/different-node-id churn. Reading the implementation against the tests, the logic checks out.
  • Nice defensive touches: pagination guards against repeated cursors and count drift mid-page, redundant production-readiness vs workflow:production-readiness label cohorts are cross-validated, and priority labels are prevented from colliding with role-label namespaces.

Minor, non-blocking notes:

  • scripts/ has no __init__.py; from scripts import get_next_production_readiness_issue relies on implicit namespace packages plus python -m unittest ... prepending CWD to sys.path. This works given how just invokes it (CWD = justfile's directory = repo root), but it's a bit implicit — worth a one-line comment or an __init__.py if this ever gets invoked a different way (e.g. pytest from a subdirectory).
  • validate_workflow_membership truncates its mismatch list to [:10] issue numbers without indicating truncation happened (no "…and N more"). Minor UX nit for a large-drift error message.
  • I wasn't able to execute the test suite in this review environment (sandbox denied python3 -m unittest / just execution), so this is a static read-through, not a confirmed green run. Worth confirming CI is actually green on this PR (the new issue-selector job in ci.yml should cover it).

CI / tooling

  • The new issue-selector job in ci.yml pins third-party actions by commit SHA with version comments and scopes permissions: contents: read — consistent with the least-privilege concerns the audit itself raises about other workflows.
  • justfile and .gitignore additions are small and appropriately scoped.

Audit documents

  • audits/2026-07-25-production-readiness-audit.md and audits/production-readiness-reassessment-playbook.md are large but well-structured (clear headers, severity rubric, reproducible gate procedures). I spot-checked a couple of claims against the actual code (e.g. the "malformed opening marker can disable verification" finding lines up with the lenient attribute/quote handling in src/parser.rs's has_ignore_true_attribute), and they appear grounded in real behavior rather than speculative.
  • AGENTIC_NAVIGATION_GUIDE.md was updated to list the new audits/, justfile, scripts/, and tests/ entries, consistent with this repo's own CLAUDE.md requirement to keep the guide accurate.
  • Per CLAUDE.md's "Documentation Alignment" rule, no README.md update was needed here since no user-facing tool behavior changed — the new selector is purely an internal remediation-workflow aid, and that's documented in CLAUDE.md itself instead.

Overall

No bugs or security issues found in the diff. The main risk with a PR this size is reviewer fatigue glossing over the ~5,300 added lines, but the structure (small code surface, heavily tested; large surface is prose) makes that tractable. Nothing here blocks merge from a code-quality standpoint.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review

This PR adds no changes to the Rust product code (src/) — it's purely an audit/planning record plus a new standalone Python tooling surface (scripts/get_next_production_readiness_issue.py, its test suite, a justfile, and a CI job to run both). That matches the PR description's claim of "no product runtime behavior change," which I confirmed by diffing the full file list.

Code quality / best practices

  • scripts/get_next_production_readiness_issue.py is unusually well structured for a repo-internal helper script: clean separation between pure decision logic (select_next, _leaf_blockers_satisfied, _gate_requirements_landed) and I/O (GitHubClient), frozen dataclasses for all domain objects, and an Enum for status/kind instead of magic strings.
  • Every GraphQL connection response is checked for totalCount == len(nodes) before use, guarding against silent pagination truncation — a detail that's easy to skip and often the source of subtle bugs in scripts like this.
  • One thing worth a maintainer gut-check: at ~1,400 lines (plus ~1,300 lines of tests) for "print the next actionable issue," this is a lot of machinery — multi-snapshot staleness re-validation (get_selection's 4-snapshot loop), transitive covered-leaf sequencing, redundant-label cohort validation, etc. The PR body explains the reasoning (fail-closed guarantees for something that will gate merge/release ordering), and the tests back the complexity up, but it's worth confirming the team is fine maintaining a script of this size for what's currently a single-repo, single-maintainer workflow.

Potential bugs

  • Nothing concrete found. The retry/staleness logic in get_selection (lines around previous_candidate/stale_selection_retries) is the most intricate part of the script — worth double-checking the test suite explicitly exercises "state changes between snapshot 1 and 2, then stabilizes" and "state never stabilizes within 4 snapshots" (the two branches that are hardest to eyeball for correctness).
  • justfile's test-production-readiness-selector recipe runs python3 -m unittest discover -s tests -p '...' without -t (top-level dir). This works today because python3 -m always prepends the CWD to sys.path, so from scripts import ... in the test module resolves correctly as long as just is invoked from the repo root — but it's a bit fragile/implicit. Not a bug as shipped, just something that could break quietly if the recipe or working-directory assumptions change later.

Security

  • No shell=True, eval/exec, or string-built shell commands — all gh invocations use argument lists via subprocess.run, so there's no command-injection surface even though several fields (repo names, label names) flow into constructed error messages/arguments.
  • The new CI job scopes permissions: contents: read explicitly at the job level, which is good least-privilege practice and consistent with the audit doc's own recommendation elsewhere in this PR about minimizing workflow permissions.
  • Nice touch: _default_command_runner gives a clear WorkflowError if gh isn't installed/authenticated instead of an opaque traceback.

Performance

  • Not a concern — this is a low-frequency, human/CI-invoked script over small (dozens of issues) GraphQL result sets, with pagination handled correctly (100-node pages, cursor-repeat detection to avoid infinite loops).

Test coverage

  • The test fixtures (_issue, _raw_issue, _requirement helpers) are clean and make the 60+ test cases easy to read. Coverage of tie-breaking (priority → leaf-before-gate → issue number), exclusion handling, and taxonomy-mismatch fail-closed paths all look present from the sample I reviewed.
  • Given the "fail closed on inconsistency" design goal, it'd strengthen confidence to see (if not already present) a test where blockedBy/subIssues pagination truncation is simulated (totalCount mismatch) to confirm the WorkflowError path is actually hit, not just the JSON/GraphQL-error paths.

Everything else

  • AGENTIC_NAVIGATION_GUIDE.md and CLAUDE.md were both updated to reflect the new files, in line with this repo's own documentation-alignment rule.
  • The audit/playbook/goal markdown files are long but read as genuinely commit-specific evidence rather than generic boilerplate, and the PR explicitly scopes them as historical records rather than living docs.

Overall: solid, careful engineering for what it is. My main ask is just a maintainer-level sanity check on whether this much selector machinery is the right long-term investment versus a simpler heuristic, since none of it touches the actual navigation-guide tool's correctness.

@plx
plx marked this pull request as ready for review July 25, 2026 21:13
@plx
plx merged commit 34a389f into main Jul 25, 2026
10 checks passed
@plx
plx deleted the plx/dhaka branch July 25, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discovered-by-codex Issues discovered via review by codex. documentation Improvements or additions to documentation production-readiness Part of the audited path from prototype to production-ready use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant