Skip to content

refactor: promote fd_real_path to pinned_fs and document dev-mode grant - #7224

Closed
aniruddhaadak80 wants to merge 1 commit into
kirodotdev:mainfrom
aniruddhaadak80:docs/app-kit-dev-mode-grant-followups
Closed

refactor: promote fd_real_path to pinned_fs and document dev-mode grant#7224
aniruddhaadak80 wants to merge 1 commit into
kirodotdev:mainfrom
aniruddhaadak80:docs/app-kit-dev-mode-grant-followups

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

Problem / Motivation

PR #6854 landed the dev-mode operator grant (.dev-grants.json, sensitive-root
refusal, re-toggle workflow) but deferred three follow-ups: the
docs/app-kit/api-reference.md contract still described installed.json:dev
as the only source of truth and recommended a per-file symlink, and the
private helper _fd_real_path had a third cross-module import.

Why it matters

  • Operators reading api-reference.md would still treat installed.json:dev
    as sufficient for an out-of-install ui/ and would try a per-file symlink
    that now answers 404 (pinned O_NOFOLLOW open). A sensitive ui root
    would look grantable.
  • _fd_real_path's Windows fail-closed containment belongs with the pinning
    helpers, not in hooks.py; three importers reaching into a private symbol
    is the drift the issue flags.

What changed (motivation → approach → change)

  • Docs — docs/app-kit/api-reference.md: replace the "symlink your source
    tree" paragraph and the four-bullet contract with a directory-symlink
    recommendation and six bullets: installed.json:dev, the new operator
    grant
    (.dev-grants.json mapping app -> realpath(ui) written only by
    the toggle, bound to the specific resolved directory, self-invalidating on
    repoint, 400 dev_grant_mismatch until re-toggle, prune-only reconcile),
    the sensitive-root refusal, plus endpoint / websocket / CLI. Clarify that
    .dev-apps.json remains an internal cache while .dev-grants.json is the
    authorization half for out-of-install roots (re-toggle after repointing
    workflow included).
  • Refactor — src/kiro_crew/pinned_fs.py: add public fd_real_path(fd)
    (same Windows GetFinalPathNameByHandleW / Linux /proc/self/fd / macOS
    F_GETPATH implementation) and export it in __all__.
  • Repoint — src/kiro_crew/hooks.py: delete the private definition,
    re-export from kiro_crew.pinned_fs import fd_real_path as _fd_real_path
    at top (keeps hooks._fd_real_path importable for existing callers/tests).
  • Repoint — src/kiro_crew/apps/builtins/spec_builder/backend/routes.py:
    import _fd_real_path from pinned_fs (split from the safe_read_file
    import) with the same None fallback.

No new behavior for in-install ui/ roots; no change to the toggle's
gateway-auth model (explicit operator confirmation for escaping grants is the
remaining item from #6907 and is left for a follow-up).

Tests

  • pytest test/test_app_dev_mode.py — 31 passed
  • pytest test/test_app_ui_file_route.py — 40 passed, 4 skipped
  • isort / flake8 on the three touched Python files — PASS
  • fd_real_path re-export: pinned_fs.fd_real_path is hooks._fd_real_path
    and spec_builder.routes._fd_real_path is pinned_fs.fd_real_path

Manual verification

Screenshots / video

N/A — docs + internal refactor, no UI.

Related Issues

Part of #6907 (docs + _fd_real_path promotion; operator-confirmation
hardening remains as a follow-up).

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@aniruddhaadak80
aniruddhaadak80 requested a review from a team as a code owner August 31, 2026 06:16
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed dd950e954996afdcbf0f5f083acb89767557291a via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/pinned_fs.py:353 -- function-local "import ctypes", "import msvcrt", and "import fcntl" violate top-level-imports -> Fix: move them to module-scope platform guards. (origin: validation)
[GPT-REVIEWED] dd950e9

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed dd950e954996afdcbf0f5f083acb89767557291a via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] dd950e9

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of dd950e954996afdcbf0f5f083acb89767557291a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verification complete. The helper promotion is sound (pinned_fs has no intra-package imports, so hooks re-importing from it is safe), and most of the new docs match dev_mode.py's shipped behavior (realpath-bound grant, toggle-only writes, prune-only reconcile, sensitive-root refusal). Two things don't hold up, detailed below.

Design-Verdict: CONCERNS

Sound refactor shape, but the new contract docs promise a dev_grant_mismatch error code no code emits, and the promotion repoints only one of three importers.

Watch

  • Phantom API surface in the contract doc. The added bullet promises "the UI route answers 400 with code: dev_grant_mismatch", but a grant mismatch returns the generic 400 {"error": "invalid path"} with no code field at all (src/kiro_crew/apps/routes.py:2604:2742), identical to every other containment refusal — dev_grant_mismatch appears nowhere in src/, test/, or docs/. Tooling or operators keying the documented re-toggle workflow off that code will never match it. Either document the actual response or land the code field in code first — an api-reference PR must not invent contract.
  • The stated drift is half-fixed. The description names three importers of the private _fd_real_path as the problem, but only spec_builder is repointed: sandbox.py:617 (whose comment explicitly cites docs+hardening follow-ups from #6854: app-kit dev-mode grant doc, toggle operator confirmation, promote _fd_real_path #6907's promotion as pending, and whose function-local import exists only to dodge a circularity the move now removes) and apps/routes.py:128 still reach into hooks._fd_real_path. Repoint both to pinned_fs.fd_real_path here; the hooks re-export then serves only external/test compat instead of keeping the drift alive.

[DESIGN-REVIEWED] dd950e9

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of dd950e954996afdcbf0f5f083acb89767557291a via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts are verified. I have everything needed to write the review.

First-Principles-Verdict: CONCERNS

The docs half promises a code: dev_grant_mismatch error body that zero lines of code emit, and the promotion repoints only 1 of the 3 importers it cites as its cause.

What this change ships

Intent: finish PR #6854's deferred follow-ups — make the dev-mode docs describe the shipped grant behavior and give _fd_real_path a public home. FIX (docs drift + a refactor recorded in issue #6907).

  1. Docs warn per-file symlinks now answer 404; recommend a directory symlink — justified.
  2. Docs describe the .dev-grants.json operator grant and re-toggle workflow — justified, but promises an error code that does not exist.
  3. Docs describe the sensitive-root toggle refusal — justified (dev_mode.py:356).
  4. Endpoint bullet adds the sensitive-root 400 — justified.
  5. Cost-model paragraph now names the internal sentinel file .dev-apps.json — declared, but self-contradictory (see Subtractions).
  6. fd_real_path is public in pinned_fs — justified; derived from docs+hardening follow-ups from #6854: app-kit dev-mode grant doc, toggle operator confirmation, promote _fd_real_path #6907, recorded at sandbox.py:616.
  7. hooks._fd_real_path becomes a re-export; both spellings stay live — declared; second spelling kept only for the two importers this PR leaves unfixed.
  8. spec_builder imports the helper from pinned_fs — justified.

Watch

  • Documented contract surface with zero emitters. The new grant bullet promises "400 with code: dev_grant_mismatch until you re-toggle". Grep dev_grant_mismatch: 0 occurrences repo-wide; the route answers a grant mismatch with {"error": "invalid path"} and no code field (apps/routes.py:2742 via _open_ui_file's "invalid" at :2604). The fix's own stated harm is operators misled by the doc; this line re-creates it for anyone matching on that code.
  • Point patch on the promotion's own stated cause. The description says "three importers reaching into a private symbol is the drift the issue flags", yet after this diff 2 of 3 still import hooks._fd_real_path (grep from kiro_crew.hooks import _fd_real_path: sandbox.py:617, apps/routes.py:128 — the latter already imports from pinned_fs on the adjacent line 129, so the repoint is one line). sandbox.py:613-616's comment "issue docs+hardening follow-ups from #6854: app-kit dev-mode grant doc, toggle operator confirmation, promote _fd_real_path #6907 tracks promoting it to a shared home" becomes false in the very commit that promotes it.

Subtractions

  • Shrink the grant bullet's "400 with code: dev_grant_mismatch" to "400" — nothing emits that code (0 hits repo-wide; apps/routes.py:2742).
  • Delete the two remaining from kiro_crew.hooks import _fd_real_path imports (sandbox.py:617, apps/routes.py:128) in favor of pinned_fs.fd_real_path, and delete sandbox.py:613-616's now-false circular-import/docs+hardening follow-ups from #6854: app-kit dev-mode grant doc, toggle operator confirmation, promote _fd_real_path #6907 comment — pinned_fs imports neither module, so the workaround's reason is gone.
  • Drop the literal filename .dev-apps.json from the cost-model paragraph — the same sentence says its "path, name, and format are internal implementation details, may change without notice"; naming it invites the dependence the paragraph forbids.

[FIRST-PRINCIPLES-REVIEWED] dd950e9

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 31, 2026
Move the Windows fail-closed descriptor-path helper out of hooks.py
into pinned_fs as public `fd_real_path` (its containment belongs with
the pinning helpers). Re-export it from hooks for existing importers
and repoint spec_builder to the new location. Document the
out-of-install `ui` grant contract in docs/app-kit/api-reference.md:
`.dev-grants.json`, sensitive-root refusal, and the re-toggle after
repointing workflow.

Part of kirodotdev#6907
@aniruddhaadak80
aniruddhaadak80 force-pushed the docs/app-kit-dev-mode-grant-followups branch from dd950e9 to a9a0632 Compare August 31, 2026 09:08
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Closing — the same two follow-ups as #7169, done less completely

Verified relationship: functional overlap

Both PRs are open (verified live: 7169 head d28aa15, 7224 head a9a0632, neither merged, both mergeable_state=dirty) and both implement the same two deferred items of open issue #6907 off the identical base blobs. Verified in the diffs: both add the same fd_real_path(fd) to src/kiro_crew/pinned_fs.py with the same three platform routes plus the same "fd_real_path", all entry, both delete the same 53-line _fd_real_path block from hooks.py (base blob 55b4155443 on both sides) and replace it with the same aliasing top-level import, both repoint spec_builder's importer, and both rewrite the same 'App Dev Mode' section of docs/app-kit/api-reference.md from base blob 342a75a8cc at base line 760 with the same claims (directory-vs-per-file symlink under O_NOFOLLOW, the .dev-grants.json operator grant bound to the realpath at toggle time, toggle-only writes / prune-only reconcile, self-invalidation on repoint, re-toggle to re-bind, the sensitive-root refusal, the added endpoint 400). That section is still the pre-#6854 text on origin/main, so this is the same prose authored twice, not a hub-file co-edit. The relationship is asymmetric: 7224's own body defers 'explicit operator confirmation for escaping grants' to a follow-up, and that follow-up is exactly what 7169 additionally ships (set_dev_mode's confirm_out_of_install_root kwarg with a validate-before-write refusal code, the CLI flag + allow_abbrev=False, cli_commands forwarding, the self-protection-dev-mode-out-of-root-confirm deny rule + _is_dev_mode_out_of_root_confirm argv floor, the SEL audit, ~400 test lines) plus the sandbox.py and apps/routes.py repoints and the location-pinning TestFdRealPathHome that 7224's own Design and First-Principles reviews demanded of it. Neither is superseded by main: main's set_dev_mode is still the two-argument signature, pinned_fs has no fd_real_path, and hooks.py still owns the private helper. Not a stacked branch: single commits off different merge-bases by different authors.

Carry this over first

This closure is about redundancy, and these items are the exception: they are not on main and not in the surviving PR, so they need a home before the topic is finished. Please don't let them go with the branch.

Two items from #7224, both small and both style/robustness tier: (1) src/kiro_crew/pinned_fs.py — the module-scope platform guards for the promoted helper instead of #7169's verbatim function-local imports: top-level import ctypes, try: import fcntl / except ImportError: fcntl = None # type: ignore[assignment], try: import msvcrt / except ImportError: msvcrt = None, and the macOS branch rewritten to if fcntl is not None and hasattr(fcntl, "F_GETPATH"); this satisfies AUTOSDE.yaml's advisory top-level-imports rule and is the fork GPT review's finding on #7224. (2) The per-module import guard split — give fd_real_path its own try/except independent of from kiro_crew.hooks import safe_read_file_bytes_nolink, so an ImportError from hooks no longer nulls the leaf-module primitive; apply it at src/kiro_crew/apps/builtins/spec_builder/backend/repository.py:41-50 (where the block now lives on main), not at backend/routes.py. Do NOT carry #7224's two unique docs phrasings: code: dev_grant_mismatch (no emitter exists — apps/routes.py returns bare 'invalid' -> a 400 with no code field) and the naming of the internal .dev-apps.json sentinel; both of #7224's reviewers asked for their removal.

Current state

Neither side has merged, so no supersession applies. Parent PR #6854 is merged and landed the grant machinery (.dev-grants.json, _read/_write_dev_grants, the sensitive-root refusal, dev_mode_granted_root) but nothing either PR adds; #6907 is an ISSUE and covers nothing. Main has, however, moved under both of them: the spec_builder fd_real_path guard migrated from backend/routes.py to backend/repository.py, and sandbox.py gained a SECOND lazy from kiro_crew.hooks import _fd_real_path (main sandbox.py:7172) carrying the same now-false '#6907 tracks promoting it' comment, which neither PR repoints. Whichever survives owes both of those on rebase.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. Findings that implied a closure were re-adjudicated independently, including an adversarial pass whose only job was to refute them; the reasoning above is what survived. If it is wrong, reopening costs nothing — please say so, and treat the reasoning rather than the outcome as the thing to correct.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • PR #7169 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7169: KEEP. PR #7224 was closed unmerged on 2026-09-02 explicitly in favour of this PR (audit note on this thread, and the cross-referenced event in timeline.json). PR #7169 subsumes everything PR #7224 carried, so nothing is lost and there is no live duplicate to reconcile. Files: src/kiro_crew/pinned_fs.py, docs/app-kit/api-reference.md, src/kiro_crew/apps/dev_mode.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants